[PATCH v2] c: Silently ignore pragma region [PR85487]

[PATCH v2] c: Silently ignore pragma region [PR85487]

[Austin Morton]

#pragma region is a feature introduced by Microsoft in order to allow
manual grouping and folding of code within Visual Studio.  It is
entirely ignored by the compiler.  Clang has supported this feature
since 2012 when in MSVC compatibility mode, and enabled it across the
board in 2018.

As it stands, you cannot use #pragma region within GCC without
disabling unknown pragma warnings, which is not advisable.

I propose GCC adopt "#pragma region" and "#pragma endregion" in order
to alleviate these issues.  Because the pragma has no purpose at
compile time, the implementation is trivial.


Microsoft Documentation on the feature:
https://docs.microsoft.com/en-us/cpp/preprocessor/region-endregion

LLVM change which enabled pragma region across the board:
https://reviews.llvm.org/D42248
---
 gcc/ChangeLog                        |  5 +++++
 gcc/c-family/ChangeLog               |  5 +++++
 gcc/c-family/c-pragma.c              | 10 ++++++++++
 gcc/doc/cpp.texi                     |  6 ++++++
 gcc/testsuite/ChangeLog              |  5 +++++
 gcc/testsuite/gcc.dg/pragma-region.c | 21 +++++++++++++++++++++
 6 files changed, 52 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pragma-region.c

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9db853dcd..d0ba77b55 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2020-09-02  Austin Morton  <austinpmorton@gmail.com>
+
+ PR c/85487
+ * doc/cpp.texi (Pragmas): Document pragma region/endregion
+
 2020-08-26  Göran Uddeborg  <goeran@uddeborg.se>

  PR gcov-profile/96285
diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog
index 1eaa99f31..ccf06095f 100644
--- a/gcc/c-family/ChangeLog
+++ b/gcc/c-family/ChangeLog
@@ -1,3 +1,8 @@
+2020-09-02  Austin Morton  <austinpmorton@gmail.com>
+
+ PR c/85487
+ * c-pragma.c (handle_pragma_region): Declare.
+
 2020-08-11  Jakub Jelinek  <jakub@redhat.com>

  PR c/96545
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index e3169e68f..de0411d07 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1166,6 +1166,13 @@ handle_pragma_message (cpp_reader *ARG_UNUSED(dummy))
     TREE_STRING_POINTER (message));
 }

+/* Silently ignore region pragmas.  */
+
+static void
+handle_pragma_region (cpp_reader *ARG_UNUSED(dummy))
+{
+}
+
 /* Mark whether the current location is valid for a STDC pragma.  */

 static bool valid_location_for_stdc_pragma;
@@ -1584,6 +1591,9 @@ init_pragma (void)

   c_register_pragma_with_expansion (0, "message", handle_pragma_message);

+  c_register_pragma (0, "region", handle_pragma_region);
+  c_register_pragma (0, "endregion", handle_pragma_region);
+
 #ifdef REGISTER_TARGET_PRAGMAS
   REGISTER_TARGET_PRAGMAS ();
 #endif
diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi
index 33f876ab7..c868ed695 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -3789,6 +3789,12 @@ file will never be read again, no matter what.
It is a less-portable
 alternative to using @samp{#ifndef} to guard the contents of header files
 against multiple inclusions.

+@item #pragma region
+@itemx #pragma endregion
+These pragmas are silently ignored by the compiler.  Any trailing text is
+also silently ignored.  They exist only to facilitate code organization
+and folding in supported editors.
+
 @end ftable

 @node Other Directives
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 5c1a45716..d90067555 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-09-02  Austin Morton  <austinpmorton@gmail.com>
+
+ PR c/85487
+ * gcc.dg/pragma-region.c: New test.
+
 2020-08-26  Jeff Law  <law@redhat.com>

  * gcc.target/i386/387-7.c: Add dg-require-effective-target c99_runtime.
diff --git a/gcc/testsuite/gcc.dg/pragma-region.c
b/gcc/testsuite/gcc.dg/pragma-region.c
new file mode 100644
index 000000000..72cc2c144
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pragma-region.c
@@ -0,0 +1,21 @@
+/* Verify #pragma region and #pragma endregion do not emit warnings.  */
+
+/* { dg-options "-Wunknown-pragmas" } */
+
+#pragma region
+
+#pragma region name
+
+#pragma region "name"
+
+#pragma region()
+
+#pragma region("name")
+
+#pragma endregion
+
+#pragma endregion garbage
+
+#pragma endregion()
+
+#pragma endregion("garbage")
-- 
2.17.1

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Jeff Law]

On 9/2/20 6:59 PM, Austin Morton via Gcc-patches wrote:
> #pragma region is a feature introduced by Microsoft in order to allow
> manual grouping and folding of code within Visual Studio.  It is
> entirely ignored by the compiler.  Clang has supported this feature
> since 2012 when in MSVC compatibility mode, and enabled it across the
> board in 2018.
>
> As it stands, you cannot use #pragma region within GCC without
> disabling unknown pragma warnings, which is not advisable.
>
> I propose GCC adopt "#pragma region" and "#pragma endregion" in order
> to alleviate these issues.  Because the pragma has no purpose at
> compile time, the implementation is trivial.
>
>
> Microsoft Documentation on the feature:
> https://docs.microsoft.com/en-us/cpp/preprocessor/region-endregion
>
> LLVM change which enabled pragma region across the board:
> https://reviews.llvm.org/D42248
> ---
>  gcc/ChangeLog                        |  5 +++++
>  gcc/c-family/ChangeLog               |  5 +++++
>  gcc/c-family/c-pragma.c              | 10 ++++++++++
>  gcc/doc/cpp.texi                     |  6 ++++++
>  gcc/testsuite/ChangeLog              |  5 +++++
>  gcc/testsuite/gcc.dg/pragma-region.c | 21 +++++++++++++++++++++
>  6 files changed, 52 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/pragma-region.c

I'm not sure that this is really the way we want to handle this stuff. 
I understand the problem you're trying to solve, but embedding a list of
pragmas to ignore into the compiler itself just seems like the wrong
approach -- it bakes that set of pragmas to ignore into the compiler.


ISTM that we'd be better off either having a command line option to list
the set of pragmas to ignore, or they should be pulled from a file
specified on the command line.   That would seem to be a lot more
friendly to downstream users since each project could set the list of
pragmas to ignore on their own and have that set updated dynamically
over time without having to patch and update GCC.


Any chance you would be willing to work on that?

Jeff

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Austin Morton]

On the contrary, as a user of GCC I would much prefer a consistent
behavior for #pragma region based purely on GCC version.

IE, so you can tell people:
    "just update to GCC X.Y and those warnings will go away"
rather than:
    "update to GCC X.Y and pass some new flags - but make sure
     not to pass them to old GCC versions, since that will generate
     a new warning"

I do agree it may be generally useful to have a configurable way to
specify pragmas to ignore at runtime, but that is not what I was trying
to accomplish here.

Both clang and MSVC handle this pragma without any runtime
configuration, and I think GCC should as well.

Austin

On Thu, Nov 12, 2020 at 11:25 PM Jeff Law <law@redhat.com> wrote:
>
>
> On 9/2/20 6:59 PM, Austin Morton via Gcc-patches wrote:
> > #pragma region is a feature introduced by Microsoft in order to allow
> > manual grouping and folding of code within Visual Studio.  It is
> > entirely ignored by the compiler.  Clang has supported this feature
> > since 2012 when in MSVC compatibility mode, and enabled it across the
> > board in 2018.
> >
> > As it stands, you cannot use #pragma region within GCC without
> > disabling unknown pragma warnings, which is not advisable.
> >
> > I propose GCC adopt "#pragma region" and "#pragma endregion" in order
> > to alleviate these issues.  Because the pragma has no purpose at
> > compile time, the implementation is trivial.
> >
> >
> > Microsoft Documentation on the feature:
> > https://docs.microsoft.com/en-us/cpp/preprocessor/region-endregion
> >
> > LLVM change which enabled pragma region across the board:
> > https://reviews.llvm.org/D42248
> > ---
> >  gcc/ChangeLog                        |  5 +++++
> >  gcc/c-family/ChangeLog               |  5 +++++
> >  gcc/c-family/c-pragma.c              | 10 ++++++++++
> >  gcc/doc/cpp.texi                     |  6 ++++++
> >  gcc/testsuite/ChangeLog              |  5 +++++
> >  gcc/testsuite/gcc.dg/pragma-region.c | 21 +++++++++++++++++++++
> >  6 files changed, 52 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.dg/pragma-region.c
>
> I'm not sure that this is really the way we want to handle this stuff.
> I understand the problem you're trying to solve, but embedding a list of
> pragmas to ignore into the compiler itself just seems like the wrong
> approach -- it bakes that set of pragmas to ignore into the compiler.
>
>
> ISTM that we'd be better off either having a command line option to list
> the set of pragmas to ignore, or they should be pulled from a file
> specified on the command line.   That would seem to be a lot more
> friendly to downstream users since each project could set the list of
> pragmas to ignore on their own and have that set updated dynamically
> over time without having to patch and update GCC.
>
>
> Any chance you would be willing to work on that?
>
> Jeff
>

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[David Malcolm]

On Fri, 2020-11-13 at 09:57 -0500, Austin Morton via Gcc-patches wrote:
> On the contrary, as a user of GCC I would much prefer a consistent
> behavior for #pragma region based purely on GCC version.
> 
> IE, so you can tell people:
>     "just update to GCC X.Y and those warnings will go away"
> rather than:
>     "update to GCC X.Y and pass some new flags - but make sure
>      not to pass them to old GCC versions, since that will generate
>      a new warning"
> 
> I do agree it may be generally useful to have a configurable way to
> specify pragmas to ignore at runtime, but that is not what I was
> trying
> to accomplish here.
> 
> Both clang and MSVC handle this pragma without any runtime
> configuration, and I think GCC should as well.

FWIW I like the patch (but I don't think I can approve it).

How much does this pragma get used "in the wild"?

Thinking aloud, I wonder if it would be useful to capture regions in
the diagnostic subsystem, and emit "In region <FOO>..." messages,
rather like we emit "In function <FOO>..." when first emitting a
diagnostic within a region?  (not sure if good idea, just
brainstorming)

Dave

> Austin
> 
> On Thu, Nov 12, 2020 at 11:25 PM Jeff Law <law@redhat.com> wrote:
> > 
> > On 9/2/20 6:59 PM, Austin Morton via Gcc-patches wrote:
> > > #pragma region is a feature introduced by Microsoft in order to
> > > allow
> > > manual grouping and folding of code within Visual Studio.  It is
> > > entirely ignored by the compiler.  Clang has supported this
> > > feature
> > > since 2012 when in MSVC compatibility mode, and enabled it across
> > > the
> > > board in 2018.
> > > 
> > > As it stands, you cannot use #pragma region within GCC without
> > > disabling unknown pragma warnings, which is not advisable.
> > > 
> > > I propose GCC adopt "#pragma region" and "#pragma endregion" in
> > > order
> > > to alleviate these issues.  Because the pragma has no purpose at
> > > compile time, the implementation is trivial.
> > > 
> > > 
> > > Microsoft Documentation on the feature:
> > > https://docs.microsoft.com/en-us/cpp/preprocessor/region-endregion
> > > 
> > > LLVM change which enabled pragma region across the board:
> > > https://reviews.llvm.org/D42248
> > > ---
> > >  gcc/ChangeLog                        |  5 +++++
> > >  gcc/c-family/ChangeLog               |  5 +++++
> > >  gcc/c-family/c-pragma.c              | 10 ++++++++++
> > >  gcc/doc/cpp.texi                     |  6 ++++++
> > >  gcc/testsuite/ChangeLog              |  5 +++++
> > >  gcc/testsuite/gcc.dg/pragma-region.c | 21 +++++++++++++++++++++
> > >  6 files changed, 52 insertions(+)
> > >  create mode 100644 gcc/testsuite/gcc.dg/pragma-region.c
> > 
> > I'm not sure that this is really the way we want to handle this
> > stuff.
> > I understand the problem you're trying to solve, but embedding a
> > list of
> > pragmas to ignore into the compiler itself just seems like the
> > wrong
> > approach -- it bakes that set of pragmas to ignore into the
> > compiler.
> > 
> > 
> > ISTM that we'd be better off either having a command line option to
> > list
> > the set of pragmas to ignore, or they should be pulled from a file
> > specified on the command line.   That would seem to be a lot more
> > friendly to downstream users since each project could set the list
> > of
> > pragmas to ignore on their own and have that set updated
> > dynamically
> > over time without having to patch and update GCC.
> > 
> > 
> > Any chance you would be willing to work on that?
> > 
> > Jeff
> > 

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Jakub Jelinek]

On Fri, Nov 13, 2020 at 09:57:31AM -0500, Austin Morton via Gcc-patches wrote:
> On the contrary, as a user of GCC I would much prefer a consistent
> behavior for #pragma region based purely on GCC version.
> 
> IE, so you can tell people:
>     "just update to GCC X.Y and those warnings will go away"
> rather than:
>     "update to GCC X.Y and pass some new flags - but make sure
>      not to pass them to old GCC versions, since that will generate
>      a new warning"
> 
> I do agree it may be generally useful to have a configurable way to
> specify pragmas to ignore at runtime, but that is not what I was trying
> to accomplish here.
> 
> Both clang and MSVC handle this pragma without any runtime
> configuration, and I think GCC should as well.

But in that case the pragma shouldn't be ignored, but instead checked
against the various requirements.  E.g. that region is followed by a single
optional name (are there any requirements on what name can be, can it be
just a single token, can it be C/C++ keyword, etc.), guess ignore all the
tokens after endregion if it is all a comment, and verify nesting
(all region/endregion are properly paired up).

	Jakub

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Austin Morton]

> But in that case the pragma shouldn't be ignored, but instead checked
> against the various requirements.  E.g. that region is followed by a single
> optional name (are there any requirements on what name can be, can it be
> just a single token, can it be C/C++ keyword, etc.), guess ignore all the
> tokens after endregion if it is all a comment, and verify nesting
> (all region/endregion are properly paired up).

I cannot speak to the internals of the MSVC compiler, but I can say
definitively that
clang does not do any of this.  The clang handling of #pragma region
is a no-op, as
can be seen here:
https://github.com/llvm/llvm-project/blob/48b510c4bc0fe090e635ee0440e46fc176527d7e/clang/lib/Lex/Pragma.cpp#L1847

Additionally, the MSVC implementation appears to do no validation (or
at least doesn't
emit warnings that indicate that it does). https://godbolt.org/z/3zTbnn

#pragma region is purely designed for editors to assist in
code-folding (primarily
visual studio and visual studio code, although I am sure others support it).

The goal of this patch is to make GCC compatible with both clang and
MSVC handling of this
pragma, not to introduce novel functionality.

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Austin Morton]

> How much does this pragma get used "in the wild"?

A quick search on github for "#pragma region" comes back with 170k C++ results
and searching for "#pragma" comes back with 38M C++ results

Possibly not the best metric, but we can "conclude" roughly 0.45% of
open source C++
code files on github using any pragmas makes use of #pragma region.

https://github.com/search?l=C%2B%2B&q=%22%23pragma+region%22&type=Code
https://github.com/search?l=C%2B%2B&q=%22%23pragma%22&type=Code

Re: [PATCH v2] c: Silently ignore pragma region [PR85487]

[Jeff Law]

On 11/13/20 7:57 AM, Austin Morton wrote:
> On the contrary, as a user of GCC I would much prefer a consistent
> behavior for #pragma region based purely on GCC version.

You can get consistent behavior with a command line argument and it's
much more useful over time as the world changes.


>
> IE, so you can tell people:
>     "just update to GCC X.Y and those warnings will go away"
> rather than:
>     "update to GCC X.Y and pass some new flags - but make sure
>      not to pass them to old GCC versions, since that will generate
>      a new warning"

I'm aware of those benefits.  But I would still claim that embedding
knowledge of other toolchain's pragmas into GCC  itself is just plain
wrong from a design standpoint.   A flag to allow specifying pragmas to
ignore would be much more useful and gets you the same level of
consistency with a much higher degree of control and future proofing.

Being able to specify them in a file would be even better (IMHO)


I'm not going to ACK this patch.  However, I won't object if someone
else wants to ACK it.


Jeff