Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

On 03/26/2018 01:19 PM, Nathan Sidwell wrote:

On 03/25/2018 10:18 AM, Volker Reichelt wrote:


Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,14 @@
    if (fail == 1)
  {
    error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);


In addtion to Paolo's comments, the new inform doesn't need the 
indentation.  Perhaps reword it to something like


"overridden function is %qD"

I.e. a more stand-alone message than a continuation of the error.

nathan


How about the following then?
I rephrased the last three errors a little to really make them 
stand-alone errors.


Again, bootstrapped and regtested.
OK for trunk?


2018-03-27  Volker Reichelt <v.reich...@netcologne.de>

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.  Tweak wording.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258860)
+++ gcc/cp/search.c    (working copy)
@@ -1904,7 +1904,7 @@
   if (pedwarn (DECL_SOURCE_LOCATION (overrider), 0,
        "invalid covariant return type for %q#D", overrider))
     inform (DECL_SOURCE_LOCATION (basefn),
-            "  overriding %q#D", basefn);
+            "overridden function is %q#D", basefn);
 }
   else
 fail = 2;
@@ -1918,12 +1918,14 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %q#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %q#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1940,8 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %q#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1953,8 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %q#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1974,21 +1978,26 @@
 {
   if (DECL_DELETED_FN (overrider))
 {
-      error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      error ("deleted function %q+D overriding non-deleted function",
+         overrider);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %qD", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
-      error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      error ("non-deleted function %q+D overriding deleted function",
+         overrider);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overridden function is %qD", basefn);
 }
   return 0;
 }
   if (DECL_FINAL_P (basefn))
 {
-  error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+  error ("virtual function %q+D overriding final function", overrider);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overridden function is %qD", basefn);
   return 0;
 }
   return 1;
===

2018-03-27  Volker Reichelt <v.reich...@netcologne.de>

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.  Adjust for new wording.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/cpp1y/auto-fn18.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: Likewise.
    * g++.dg/inherit/cov

Re: [C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

On 03/25/2018 08:48 PM, Paolo Carlini wrote:

Hi Volker


On 25 Mar 2018, at 16:18, Volker Reichelt <v.reich...@netcologne.de> wrote:

Hi,

when overriding a virtual function fails, the C++ front-end usually
emits two errors: one for the override that fails and one for the
function that is overridden. The second error is confusing and
should be replaced by a note to be in line with other diagnostics.

The attached patch just does that: It replaces the error/error pattern
by an error/inform pattern in search.c (check_final_overrider). And it
replaces the affected dg-error marks in the testsuite by dg-message.

Since you are correctly using DECL_SOURCE_LOCATION, I would recommend removing 
the ‘+’ modifiers, which in that case are redundant anyway and are known be a 
little cryptic and even causing tricky bugs together with warnings.

Cheers,
Paolo

Thanks, Paolo.
Below is an updated patch without the redundant "+" modifiers.

On 03/26/2018 12:33 AM, Paolo Carlini wrote:

On 25/03/2018 21:08, Paolo Carlini wrote:
... oh, please also double check that with 'F' you don't need the 
general location_of instead of D_S_L, which normally goes with 'D' - 
I don't have my machines at hand to do it myself, sorry.

Just checked, DECL_SOURCE_LOCATION is fine.

Paolo.


I also checked that DECL_SOURCE_LOCATION with %qF is OK.
In all the other places in the C++ frontend where %q#F or %qF is
used, DECL_SOURCE_LOCATION() is used for the corresponding argument:

  call.c (joust)
  decl.c (wrapup_namespace_globals)
  decl.c (check_redeclaration_exception_specification)
  method.c (maybe_explain_implicit_delete)

I also checked the output for that specific case manually.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2018-03-25  Volker Reichelt  <v.reich...@netcologne.de>

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,12 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1938,7 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1950,7 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1975,13 +1975,15 @@
   if (DECL_DELETED_FN (overrider))
 {
   error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding non-deleted function %qD", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
   error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding deleted function %qD", basefn);
 }
   return 0;
 }
@@ -1988,7 +1990,8 @@
   if (DECL_FINAL_P (basefn))
 {
   error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn),
+      "overriding final function %qD", basefn);
   return 0;
 }
   return 1;
===

2018-03-25  Volker Reichelt  <v.reich...@netcologne.de>

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: L

Re: Errors in pairs

[Volker Reichelt]

On 03/24/2018 10:35 PM, Paolo Carlini wrote:

Hi,

On 24/03/2018 21:06, Volker Reichelt wrote:

Hi everybody,

while bug-hunting I noticed that we emit lots of erros in pairs in
check_final_overrider (cp/search.c), e.g.:

  error ("invalid covariant return type for %q+#D", overrider);
  error ("  overriding %q+#D", basefn);

I would expect the second line to be emitted as a note (using inform).
Is there a reason for this or should that be changed?
IMHO that should be changed. As you may or may not have noticed, 
personally I took care of many of such issues, but obviously not of 
this one (I'm not surprised, I remember thinking a few times "I should 
come back to this one too..."). I encourage you to send patches!


Paolo.



OK, here we go:

https://gcc.gnu.org/ml/gcc-patches/2018-03/msg01364.html

Regards,
Volker

[C++ Patch] Fix confusing diagnostics for invalid overrides

[Volker Reichelt]

Hi,

when overriding a virtual function fails, the C++ front-end usually
emits two errors: one for the override that fails and one for the
function that is overridden. The second error is confusing and
should be replaced by a note to be in line with other diagnostics.

The attached patch just does that: It replaces the error/error pattern
by an error/inform pattern in search.c (check_final_overrider). And it
replaces the affected dg-error marks in the testsuite by dg-message.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2018-03-25  Volker Reichelt <v.reich...@netcologne.de>

    * search.c (check_final_overrider): Use inform instead of error
    for the diagnostics of the overridden functions.

Index: gcc/cp/search.c
===
--- gcc/cp/search.c    (revision 258835)
+++ gcc/cp/search.c    (working copy)
@@ -1918,12 +1918,14 @@
   if (fail == 1)
 {
   error ("invalid covariant return type for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);
 }
   else
 {
   error ("conflicting return type specified for %q+#D", overrider);
-      error ("  overriding %q+#D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "  overriding %q+#D", basefn);
 }
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
@@ -1938,7 +1940,7 @@
   if (!comp_except_specs (base_throw, over_throw, ce_derived))
 {
   error ("looser throw specifier for %q+#F", overrider);
-  error ("  overriding %q+#F", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q+#F", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1950,7 +1952,7 @@
   && !tx_safe_fn_type_p (over_type))
 {
   error ("conflicting type attributes specified for %q+#D", 
overrider);

-  error ("  overriding %q+#D", basefn);
+  inform (DECL_SOURCE_LOCATION (basefn), "  overriding %q+#D", basefn);
   DECL_INVALID_OVERRIDER_P (overrider) = 1;
   return 0;
 }
@@ -1975,13 +1977,15 @@
   if (DECL_DELETED_FN (overrider))
 {
   error ("deleted function %q+D", overrider);
-      error ("overriding non-deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding non-deleted function %q+D", basefn);
   maybe_explain_implicit_delete (overrider);
 }
   else
 {
   error ("non-deleted function %q+D", overrider);
-      error ("overriding deleted function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+          "overriding deleted function %q+D", basefn);
 }
   return 0;
 }
@@ -1988,7 +1992,8 @@
   if (DECL_FINAL_P (basefn))
 {
   error ("virtual function %q+D", overrider);
-  error ("overriding final function %q+D", basefn);
+      inform (DECL_SOURCE_LOCATION (basefn),
+      "overriding final function %q+D", basefn);
   return 0;
 }
   return 1;
===

2018-03-25  Volker Reichelt <v.reich...@netcologne.de>

    * g++.dg/cpp0x/defaulted2.C: Use dg-message instead of dg-error
    for the diagnostics of overridden functions.
    * g++.dg/cpp0x/implicit1.C: Likewise.
    * g++.dg/cpp0x/override1.C: Likewise.
    * g++.dg/eh/shadow1.C: Likewise.
    * g++.dg/inherit/covariant12.C: Likewise.
    * g++.dg/inherit/covariant14.C: Likewise.
    * g++.dg/inherit/covariant15.C: Likewise.
    * g++.dg/inherit/covariant16.C: Likewise.
    * g++.dg/inherit/crash3.C: Likewise.
    * g++.dg/inherit/error2.C: Likewise.
    * g++.dg/template/crash100.C: Likewise.
    * g++.old-deja/g++.eh/spec6.C: Likewise.
    * g++.old-deja/g++.mike/p811.C: Likewise.
    * g++.old-deja/g++.other/virtual11.C: Likewise.
    * g++.old-deja/g++.other/virtual4.C: Likewise.

Index: gcc/testsuite/g++.dg/cpp0x/defaulted2.C
===
--- gcc/testsuite/g++.dg/cpp0x/defaulted2.C    (revision 258835)
+++ gcc/testsuite/g++.dg/cpp0x/defaulted2.C    (working copy)
@@ -25,7 +25,7 @@

 struct C
 {
-  virtual void f() = delete;    // { dg-error "overriding deleted" }
+  virtual void f() = delete;    // { dg-message "overriding deleted" }
 };

 struct D: public C
Index: gcc/testsuite/g++.dg/cpp0x/implicit1.C
===
--- gcc/testsuite/g++.dg/cpp0x/implicit1.C    (revision 258835)
+++ gcc/testsuite/g++.dg/cpp0x/implicit1.C    (working copy)
@@ -7,7 +7,7 @@
 {
   void operator delete (void *); // { dg-message "private" }
 public:
-  virtual 

Errors in pairs

[Volker Reichelt]

Hi everybody,

while bug-hunting I noticed that we emit lots of erros in pairs in
check_final_overrider (cp/search.c), e.g.:

  error ("invalid covariant return type for %q+#D", overrider);
  error ("  overriding %q+#D", basefn);

I would expect the second line to be emitted as a note (using inform).
Is there a reason for this or should that be changed?

Regards,
Volker

Re: [PATCH] Add a few new contrib.texi entries, especially for people who perform GCC fuzzy testing and report bugs (take 2)

[Volker Reichelt]

On 03/07/2018 03:24 PM, Jakub Jelinek wrote:


On Wed, Mar 07, 2018 at 02:06:33PM +0100, Jakub Jelinek wrote:

As appreciation of the hard work of people doing fuzzy testing of
GCC and reporting high quality bugs, I'd like to list them in contrib.texi.
This patch just lists them in the GCC testing group, shall we have a special
sub-list for the fuzzy testing?
The patch also adds or updates a couple of other entries, but I'm sure I've
missed many people in both the testing and GCC development categories, for
which I apologize.  We can always have incremental patches, contrib.texi
is helplessly obsolete anyway and for many doesn't reflect last 10-15 years
of work at all.  We should also add entries for people reporting lots of
bugs even if it is not from fuzzy testing.

Anyway, ok for trunk?  Are all people on the CC ok with them being listed in
there (reply privately if needed)?




Thanks Jakub, for the appreciation!

My way of saying thank you, is (of course) to file an new 
ice-on-valid-code regression:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84752  ;-)

Thanks again,
Volker

Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

On 23 Jul, Martin Sebor wrote:
> On 07/23/2017 02:42 PM, Volker Reichelt wrote:
>> On 21 Jul, Martin Sebor wrote:
>>> On 07/20/2017 10:35 AM, Volker Reichelt wrote:
>>>> Hi,
>>>>
>>>> the following patch introduces a new C++ warning option
>>>> -Wduplicated-access-specifiers that warns about redundant
>>>> access-specifiers in classes, e.g.
>>>>
>>>>   class B
>>>>   {
>>>> public:
>>>>   B();
>>>>
>>>> private:
>>>>   void foo();
>>>> private:
>>>>   int i;
>>>>   };
>>>
>>> I'm very fond of warnings that help find real bugs, or even that
>>> provide an opportunity to review code for potential problems or
>>> inefficiencies and suggest a possibility to improve it in some
>>> way (make it clearer, or easier for humans or compilers to find
>>> real bugs in, or faster, etc.), even if the code isn't strictly
>>> incorrect.
>>>
>>> In this case I'm having trouble coming up with an example where
>>> the warning would have this effect.  What do you have in mind?
>>> (Duplicate access specifiers tend to crop up in large classes
>>> and/or as a result of preprocessor conditionals.)
>>
>> This warning fights the "tend to crop up" effect that clutters the
>> code. After some time these redundant access specifiers creep in
>> and make your code harder to read. If I see something like
>>
>>   ...
>> void foo() const;
>>
>>   private:
>> void bar();
>>   ...
>>
>> on the screen I tend to think that 'foo' has a different access
>> level than bar. If that's not the case because the access-specifier
>> is redundant, then that's just confusing and distracting.
>> The warning helps to maintain readability of your code.
>>
>> The benefit might not be big, but the warning comes at relatively
>> low cost. It passes a location around through the class stack and
>> checks less than a handful of tokens.
>>
>> My personal motivation to implement this warning was the fact that
>> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
>> mechanism to the new function pointer syntax of Qt5. In the old
>> version you had to mark slots in the following fashion:
>>
>>   public slots:
>> void foo();
>> void bar();
>>
>> But now you can use any function making the 'slots' macro obsolete.
>> Therefore I ended up with hundreds of redundant access-specifiers
>> which this warning helped to clean up. Doing this sort of thing in the
>> compiler with a proper parser is way safer than to write some script
>> to achieve this.
> 
> Okay, thanks for clarifying that.  I think what's distracting to
> one could be helpful to another.  For example, it's not uncommon
> for classes with many members to use redundant access specifiers
> to group blocks of related declarations.  Or, in a class with many
> lines of comments (e.g., Doxygen), repeating the access specifier
> every so often could be seen as helpful because otherwise there
> would be no way to tell what its access is without scrolling up
> or down.  It's debatable what approach to dealing with this is
> best.  Java, for instance, requires every non-public member to
> be declared with its own access specifier.  Some other languages
> (I think D) do as well.  An argument could be made that that's
> a far clearer style than using the specifier only when changing
> access.  It seems to me that the most suitable approach will be
> different from one project to another, if not from one person to
> another.  A diagnostic that recommends a particular style (or that
> helps with a specific kind of a project like the one you did for
> Qt) might be a good candidate for a plugin, but enshrining any
> one style (or a solution to a project-specific problem) in GCC
> as a general-purpose warning doesn't seem appropriate or in line
> with the definition of warnings in GCC:
> 
>constructions that are not inherently erroneous but that are
>risky or suggest there may have been an error
> 
> Martin
> 
> PS There are other redundancies that some might say unnecessarily
> clutter code.  For instance, declaring a symbol static in
> an unnamed namespace, or explicitly declaring a member function
> inline that's also defined within the body of a class, or
> explicitly declaring a function virtual that overrides one
> declared in a base class.  None of these is diagnosed, and I'd
> say for good reason: they are all benign and 

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

On 25 Jul, Marek Polacek wrote:
> On Sun, Jul 23, 2017 at 11:22:14PM +0200, Volker Reichelt wrote:
> [...]
> 
> Not sure if the warning is too useful, but in any case...
> 
>> +  /* Emit warning.  */
>> +  gcc_rich_location richloc (token->location);
>> +  richloc.add_fixit_remove ();
>> +  if (colon_token->type == CPP_COLON)
>> +richloc.add_fixit_remove (colon_token->location);
>> +
>> +  switch (message_id)
>> +{
>> +case 1:
>> +  warning_at_rich_loc (, OPT_Waccess_specifiers_,
>> +   "redundant %qE access-specifier",
>> +   token->u.value);
>> +  inform (next_token->location, "directly followed by another one 
>> here");
>> +  break;
>> +
>> +case 2:
>> +  warning_at_rich_loc (, OPT_Waccess_specifiers_,
>> +   "duplicate %qE access-specifier",
>> +   token->u.value);
>> +  inform (current_access_specifier_loc,
>> +  "same access-specifier was previously given here");
>> +  break;
> 
> ...you should only call inform if warning_at_rich_loc returned true.
> 
>   Marek

I also noticed yesterday that the inform calls weren't suppressed
together with the warningss in system headers. I was thinking about
adding an in_system_header_at check, but your your suggestion is much
nicer.

Thanks,
Volker

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

On 23 Jul, Eric Gallager wrote:
> On 7/23/17, Volker Reichelt <v.reich...@netcologne.de> wrote:
>> Hi again,
>>
>> here is an updated patch for a new warning about redundant
>> access-specifiers. It takes Dave's various comments into account.
>>
>> The main changes w.r.t. to the previous versions are:
>>
>> * The warning is now a two-level warning with a slightly shorter name:
>>   -Waccess-specifiers=1, -Waccess-specifiers=2
>>   with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
> 
> Just a more generalized comment as a user, but I don't really like
> this trend that new warning options are so often given numeric levels
> these days. A warning option with different levels requires special
> handling in configure scripts or Makefiles, which is harder than just
> toggling different names (i.e. how things work without numeric
> levels).

Fair point.

>> * The warning now checks for 3 different scenarios, see testcase
>>   Waccess-specifiers-2.C below:
>>   A) Two consecutive access-specifiers, so that the first one
>>  has no effect. (This is new.)
>>   B) Two (non-consecutive) equal access-specifiers.
>>  (That's what the original patch checked.)
>>   C) An access-specifier that does not change the class-type's default.
>>  (That's what I only suggested in the original patch.)
>>   The first two tests are enabled in level 1, the last in level 2.
> 
> Instead of levels, I'd prefer A and B to be in an option like the
> original -Wduplicate-access-specifiers, while putting C under a
> separate name like -Wdefault-access-specifiers. This way they can be
> toggled individually, so for example I can get just warning C without
> also getting warnings A and B. Using numeric levels makes that
> impossible. But that's just my personal preference for how to separate
> them, so feel free to disregard me.

What I like about the numeric levels is that they are easier to
remember. You don't have to look up different names.
OTOH, toggling on C without A+B might really make sense here.

But I'd like to get some more opinions before I change that.

Regards,
Volker

[PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

Hi again,

here is an updated patch for a new warning about redundant
access-specifiers. It takes Dave's various comments into account.

The main changes w.r.t. to the previous versions are:

* The warning is now a two-level warning with a slightly shorter name:
  -Waccess-specifiers=1, -Waccess-specifiers=2
  with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
* The warning now checks for 3 different scenarios, see testcase
  Waccess-specifiers-2.C below:
  A) Two consecutive access-specifiers, so that the first one
 has no effect. (This is new.)
  B) Two (non-consecutive) equal access-specifiers.
 (That's what the original patch checked.)
  C) An access-specifier that does not change the class-type's default.
 (That's what I only suggested in the original patch.)
  The first two tests are enabled in level 1, the last in level 2.
* The warning is now in a separate function.
* The fix-it hints now include the colon after the access-specifier.
* There's a testcase for the fix-it hints.

What's missing from Dave's comments are some examples for invoke.texi.
I'll work on that once the warning made it into the trunk.

The switch statement in cp_parser_maybe_warn_access_specifier is a little
bit ugly. It would be nicer to emit the warnings directly next to the
checks. But then I'd have to write the code for the fix-it hint 3 times.
With C++11 I'd use a lambda for this, but with C++03 I hope the switch
is OK.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-07-22  Volker Reichelt  <v.reich...@netcologne.de>

* doc/invoke.texi (-Waccess-specifiers, -Waccess-specifiers=):
Document new warning options.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 250356)
+++ gcc/doc/invoke.texi (working copy)
@@ -259,7 +259,8 @@
 @xref{Warning Options,,Options to Request or Suppress Warnings}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -Wpedantic @gol
 -pedantic-errors @gol
--w  -Wextra  -Wall  -Waddress  -Waggregate-return  @gol
+-w  -Wextra  -Wall  -Waccess-specifiers  -Waccess-specifiers=@var{n} @gol
+-Waddress  -Waggregate-return  @gol
 -Walloc-zero  -Walloc-size-larger-than=@var{n}
 -Walloca  -Walloca-larger-than=@var{n} @gol
 -Wno-aggressive-loop-optimizations  -Warray-bounds  -Warray-bounds=@var{n} @gol
@@ -5254,6 +5255,13 @@
 Warn about overriding virtual functions that are not marked with the override
 keyword.
 
+@item -Waccess-specifiers @r{(C++ and Objective-C++ only)}
+@itemx -Waccess-specifiers=@var{n}
+@opindex Wno-access-specifiers
+@opindex Waccess-specifiers
+Warn when an access-specifier is redundant because it was already given
+before.
+
 @item -Walloc-zero
 @opindex Wno-alloc-zero
 @opindex Walloc-zero
===


2017-07-22  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Waccess-specifiers=): New C++ warning flag.
(Waccess-specifiers): New alias.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 250443)
+++ gcc/c-family/c.opt  (working copy)
@@ -476,6 +476,14 @@
 C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
 Warn about compile-time integer division by zero.
 
+Waccess-specifiers
+C++ ObjC++ Warning Alias(Waccess-specifiers=, 1, 0)
+Warn about redundant access-specifiers.
+
+Waccess-specifiers=
+C++ ObjC++ Var(warn_access_specifiers) Warning Joined RejectNegative UInteger
+Warn about redundant access-specifiers.
+
 Wduplicated-branches
 C ObjC C++ ObjC++ Var(warn_duplicated_branches) Init(0) Warning
 Warn about duplicated branches in if-else statements.
===


2017-07-22  Volker Reichelt  <v.reich...@netcologne.de>

* cp-tree.h (struct saved_scope): New access_specifier_loc variable.
(current_access_specifier_loc): New macro.
* class.c (struct class_stack_node): New access_loc variable.
(pushclass): Push current_access_specifier_loc.  Set new
initial value.
(popclass): Pop current_access_specifier_loc.
* parser.c (cp_parser_maybe_warn_access_specifier): New function
to warn about duplicated access-specifiers.
(cp_parser_member_specification_opt): Call it.  Set
current_access_specifier_loc.

Index: gcc/cp/class.c
===
--- gcc/cp/class.c  (revision 250443)
+++ gcc/cp/class.c  (working copy)
@@ -60,6 +60,7 @@
   /* The access specifier pending for new declarations in the scope of
  this class.  */
   tree access;
+  location_t access_loc;
 
   /* If were defining TYPE, the names used in this class.  */
   splay_tree names_used;
@@ -7724,6 +7725,7 @@
   csn->name = current_class_name;
   csn->type = current_class_type;
   csn->access

Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

On 21 Jul, David Malcolm wrote:
> On Fri, 2017-07-21 at 19:58 +0200, Volker Reichelt wrote:
>> On 21 Jul, David Malcolm wrote:
>> > On Thu, 2017-07-20 at 18:35 +0200, Volker Reichelt wrote:
>> >> Hi,
>> >> 
>> >> the following patch introduces a new C++ warning option
>> >> -Wduplicated-access-specifiers that warns about redundant
>> >> access-specifiers in classes, e.g.
>> >> 
>> >>   class B
>> >>   {
>> >> public:
>> >>   B();
>> >> 
>> >> private:
>> >>   void foo();
>> >> private:
>> >>   int i;
>> >>   };
>> >> 
>> >> test.cc:8:5: warning: duplicate 'private' access-specifier [
>> >> -Wduplicated-access-specifiers]
>> >>  private:
>> >>  ^~~
>> >>  ---
>> >> test.cc:6:5: note: access-specifier was previously given here
>> >>  private:
>> >>  ^~~

{...snip...]

>> > If you're going to implement a fix-it hint for this, there should
>> be a
>> > test case that tests it (probably as a separate file, e.g.
>> Wduplicated
>> > -access-specifiers-2.C, to allow for the extra option -
>> -fdiagnostics
>> > -show-caret, covering just one instance of the diagnostic firing,
>> for
>> > simplicity).
>> 
>> I actually did try that, but dejagnu somehow gets confused.
>> To me it looks like the reason for this is that both multi-line
>> messages
>> are so similar. I'll give it a second try, though.
> 
> I'm not sure what you mean by "both" multi-line messages; shouldn't
> there be just one multi-line message for the fix-it hint.
> 
> Isn't this like e.g. g++.dg/cpp0x/auto1.C ?  (an example of a testcase
> that verifies a deletion fix-it hint)
> 
> Dave

There are two multi-line messages. One for the warning and one for the
note, see the example above the "[...snip...]". The message after the
note consists of the first two lines of the message after the warning.
This seems to confuse dejagnu. However, I managed to work around this
problem. I'll post an updated patch soon.

Regards,
Volker

Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

On 21 Jul, Martin Sebor wrote:
> On 07/20/2017 10:35 AM, Volker Reichelt wrote:
>> Hi,
>>
>> the following patch introduces a new C++ warning option
>> -Wduplicated-access-specifiers that warns about redundant
>> access-specifiers in classes, e.g.
>>
>>   class B
>>   {
>> public:
>>   B();
>>
>> private:
>>   void foo();
>> private:
>>   int i;
>>   };
> 
> I'm very fond of warnings that help find real bugs, or even that
> provide an opportunity to review code for potential problems or
> inefficiencies and suggest a possibility to improve it in some
> way (make it clearer, or easier for humans or compilers to find
> real bugs in, or faster, etc.), even if the code isn't strictly
> incorrect.
> 
> In this case I'm having trouble coming up with an example where
> the warning would have this effect.  What do you have in mind?
> (Duplicate access specifiers tend to crop up in large classes
> and/or as a result of preprocessor conditionals.)

This warning fights the "tend to crop up" effect that clutters the
code. After some time these redundant access specifiers creep in
and make your code harder to read. If I see something like

  ...
void foo() const;

  private:
void bar();
  ...

on the screen I tend to think that 'foo' has a different access
level than bar. If that's not the case because the access-specifier
is redundant, then that's just confusing and distracting.
The warning helps to maintain readability of your code.

The benefit might not be big, but the warning comes at relatively
low cost. It passes a location around through the class stack and
checks less than a handful of tokens.

My personal motivation to implement this warning was the fact that
I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
mechanism to the new function pointer syntax of Qt5. In the old
version you had to mark slots in the following fashion:

  public slots:
void foo();
void bar();

But now you can use any function making the 'slots' macro obsolete.
Therefore I ended up with hundreds of redundant access-specifiers
which this warning helped to clean up. Doing this sort of thing in the
compiler with a proper parser is way safer than to write some script
to achieve this.

Btw, the SonarAnalyzer also provides this warning to clean up code:

https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539

> Btw., there are examples of poor coding practices where I can
> imagine a warning focused on access specifiers being helpful.
> For instance, a class whose member functions maintain non-trivial
> invariants among its data members should declare the data private
> to prevent clients from inadvertently breaking those invariants.
> A specific example might be a container class (like string or
> vector) that owns the block of memory it allocates.  A warning
> that detected when such members are publicly accessible could
> help improve encapsulation.  I suspect this would be challenging
> to implement and would likely require some non-trivial analysis
> in the middle end.

That's way beyond the scope of what my warning is trying to achieve.

> Another example is a class that defines an inaccessible member
> that isn't used (e.g., class A { int i; }; that Clang detects
> with -Wunused-private-field; or class A { void f () { } };).
> I would expect this to be doable in the front end.

That's indeed a nice warning, too.

> Martin

Regards,
Volker

Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

On 21 Jul, David Malcolm wrote:
> On Thu, 2017-07-20 at 18:35 +0200, Volker Reichelt wrote:
>> Hi,
>> 
>> the following patch introduces a new C++ warning option
>> -Wduplicated-access-specifiers that warns about redundant
>> access-specifiers in classes, e.g.
>> 
>>   class B
>>   {
>> public:
>>   B();
>> 
>> private:
>>   void foo();
>> private:
>>   int i;
>>   };
>> 
>> test.cc:8:5: warning: duplicate 'private' access-specifier [
>> -Wduplicated-access-specifiers]
>>  private:
>>  ^~~
>>  ---
>> test.cc:6:5: note: access-specifier was previously given here
>>  private:
>>  ^~~
> 
> Thanks for working on this.
> 
> I'm not able to formally review, but you did CC me; various comments below 
> throughout.
> 
>> The test is implemented by tracking the location of the last
>> access-specifier together with the access-specifier itself.
>> The location serves two purposes: the obvious one is to be able to
>> print the location in the note that comes with the warning.
>> The second purpose is to be able to distinguish an access-specifier
>> given by the user from the default of the class type (i.e. public for
>> 'struct' and private for 'class') where the location is set to
>> UNKNOWN_LOCATION. The warning is only issued if the user gives the
>> access-specifier twice, i.e. if the previous location is not
>> UNKNOWN_LOCATION.
> 
> Presumably given
> 
> struct foo
> {
> public:
>   /* ... *
> };
> 
> we could issue something like:
> 
>   warning: access-specifier 'public' is redundant within 'struct'
> 
> for the first; similarly for:
> 
> class bar
> {
> private:
> };
> 
> we could issue:
> 
>   warning: access-specifier 'private' is redundant within 'class'
> 
> 
>> One could actually make this a two-level warning so that on the
>> higher level also the default class-type settings are taken into
>> account. Would that be helpful? I could prepare a second patch for
>> that.
> 
> I'm not sure how best to structure it.
> 
> FWIW, when I first saw the patch, I wasn't a big fan of the warning, as I 
> like to use access-specifiers to break up the kinds of entities within a 
> class.
> 
> For example, our coding guidelines 
>   https://gcc.gnu.org/codingconventions.html#Class_Form
> recommend:
> 
> "first define all public types,
> then define all non-public types,
> then declare all public constructors,
> ...
> then declare all non-public member functions, and
> then declare all non-public member variables."
> 
> I find it's useful to put a redundant "private:" between the last two,
> e.g.:
> 
> class baz
> {
>  public:
>   ...
> 
>  private:
>   void some_private_member ();
> 
>  private:
>   int m_some_field;
> };
> 
> to provide a subdivision between the private member functions and the
> private data fields.

That's what also can be seen in our libstdc++ to some extent.
The other half of the warnings indicate redundant access-specifiers.

It's up to the user to keep those duplicate access-specifiers as
subdividers or to use something else (like comments) to do that
and to switch on the warning for her/his code.
Because the subdivider usage seems to be relatively common,
I don't want to enable the warning by -Wall or -Wextra.

> This might be a violation of our guidelines (though if so, I'm not sure
> it's explicitly stated), but I find it useful, and the patch would warn
> about it.
> 
> Having said that, looking at e.g. the "jit" subdir, I see lots of
> places where the warning would fire.  In some of these, the code has a
> bit of a "smell", so maybe I like the warning after all... in that it
> can be good for a new warning to draw attention to code that might need
> work.
> 
> Sorry that this is rambling; comments on the patch inline below.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?
>> 
>> Btw, there are about 50 places in our libstdc++ headers where this
>> warning triggers. I'll come up with a patch for this later.
>> 
>> Regards,
>> Volker
>> 
>> 
>> 2017-07-20  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>> * doc/invoke.texi (-Wduplicated-access-specifiers): Document
>> new
>> warning option.
>> 
>> Index: gcc/doc/invoke.texi
>> ===
>> --- gcc/doc/invoke.texi (revision 250356)
>> +++ gcc/doc/invoke.tex

Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

On 21 Jul, David Malcolm wrote:
> On Fri, 2017-07-21 at 11:56 -0400, David Malcolm wrote:
>> On Thu, 2017-07-20 at 18:35 +0200, Volker Reichelt wrote:
>> > Hi,
>> > 
>> > the following patch introduces a new C++ warning option
>> > -Wduplicated-access-specifiers that warns about redundant
>> > access-specifiers in classes, e.g.
>> > 
>> >   class B
>> >   {
>> > public:
>> >   B();
>> > 
>> > private:
>> >   void foo();
>> > private:
>> >   int i;
>> >   };
>> > 
>> > test.cc:8:5: warning: duplicate 'private' access-specifier [
>> > -Wduplicated-access-specifiers]
>> >  private:
>> >  ^~~
>> >  ---
>> > test.cc:6:5: note: access-specifier was previously given here
>> >  private:
>> >  ^~~
>> 
>> Thanks for working on this.
>> 
>> I'm not able to formally review, but you did CC me; various comments
>> below throughout.
>> 
>> > The test is implemented by tracking the location of the last
>> > access-specifier together with the access-specifier itself.
>> > The location serves two purposes: the obvious one is to be able to
>> > print the location in the note that comes with the warning.
>> > The second purpose is to be able to distinguish an access-specifier
>> > given by the user from the default of the class type (i.e. public
>> > for
>> > 'struct' and private for 'class') where the location is set to
>> > UNKNOWN_LOCATION. The warning is only issued if the user gives the
>> > access-specifier twice, i.e. if the previous location is not
>> > UNKNOWN_LOCATION.
>> 
>> Presumably given
>> 
>> struct foo
>> {
>> public:
>>   /* ... *
>> };
>> 
>> we could issue something like:
>> 
>>   warning: access-specifier 'public' is redundant within 'struct'
>> 
>> for the first; similarly for:
>> 
>> class bar
>> {
>> private:
>> };
>> 
>> we could issue:
>> 
>>   warning: access-specifier 'private' is redundant within 'class'
>> 
>> 
>> > One could actually make this a two-level warning so that on the
>> > higher level also the default class-type settings are taken into
>> > account. Would that be helpful? I could prepare a second patch for
>> > that.
>> 
>> I'm not sure how best to structure it.
> 
> Maybe combine the two ideas, and call it
>   -Wredundant-access-specifiers
> ?
> 
> Or just "-Waccess-specifiers" ?
> 
> [...snip...]
> 
> Dave

Thanks for having a look at this!

My favorite version would be to use '-Waccess-specifiers=1' for the
original warning and '-Waccess-specifiers=2' for the stricter version
that also checks against the class-type default.

We could then let '-Waccess-specifiers' default to any of those two.
I'm afraid that level 2 will fire way too often for legacy code
(and there might be coding conventions that require an access specifier
at the beginning of a class/struct). So I'd vote for level 1 as default.

The last argument is also the reason why I'd like to see a two-lveel
warning instead of just different error messages (although these are
very helpful for seeing what's really goiing on with your code).

What do you think?

Regards,
Volker

[PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

Hi,

the following patch introduces a new C++ warning option
-Wduplicated-access-specifiers that warns about redundant
access-specifiers in classes, e.g.

  class B
  {
public:
  B();

private:
  void foo();
private:
  int i;
  };

test.cc:8:5: warning: duplicate 'private' access-specifier 
[-Wduplicated-access-specifiers]
 private:
 ^~~
 ---
test.cc:6:5: note: access-specifier was previously given here
 private:
 ^~~

The test is implemented by tracking the location of the last
access-specifier together with the access-specifier itself.
The location serves two purposes: the obvious one is to be able to
print the location in the note that comes with the warning.
The second purpose is to be able to distinguish an access-specifier
given by the user from the default of the class type (i.e. public for
'struct' and private for 'class') where the location is set to
UNKNOWN_LOCATION. The warning is only issued if the user gives the
access-specifier twice, i.e. if the previous location is not
UNKNOWN_LOCATION.

One could actually make this a two-level warning so that on the
higher level also the default class-type settings are taken into
account. Would that be helpful? I could prepare a second patch for that.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Btw, there are about 50 places in our libstdc++ headers where this
warning triggers. I'll come up with a patch for this later.

Regards,
Volker


2017-07-20  Volker Reichelt  <v.reich...@netcologne.de>

* doc/invoke.texi (-Wduplicated-access-specifiers): Document new
warning option.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 250356)
+++ gcc/doc/invoke.texi (working copy)
@@ -275,7 +275,7 @@
 -Wdisabled-optimization @gol
 -Wno-discarded-qualifiers  -Wno-discarded-array-qualifiers @gol
 -Wno-div-by-zero  -Wdouble-promotion @gol
--Wduplicated-branches  -Wduplicated-cond @gol
+-Wduplicated-access-specifiers  -Wduplicated-branches  -Wduplicated-cond @gol
 -Wempty-body  -Wenum-compare  -Wno-endif-labels  -Wexpansion-to-defined @gol
 -Werror  -Werror=*  -Wextra-semi  -Wfatal-errors @gol
 -Wfloat-equal  -Wformat  -Wformat=2 @gol
@@ -5388,6 +5388,12 @@
 
 This warning is enabled by @option{-Wall}.
 
+@item -Wduplicated-access-specifiers
+@opindex Wno-duplicated-access-specifiers
+@opindex Wduplicated-access-specifiers
+Warn when an access-specifier is redundant because it was already given
+before.
+
 @item -Wduplicated-branches
 @opindex Wno-duplicated-branches
 @opindex Wduplicated-branches
===


2017-07-20  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Wduplicated-access-specifiers): New C++ warning flag.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 250356)
+++ gcc/c-family/c.opt  (working copy)
@@ -476,6 +476,10 @@
 C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
 Warn about compile-time integer division by zero.
 
+Wduplicated-access-specifiers
+C++ ObjC++ Var(warn_duplicated_access) Warning
+Warn about duplicated access-specifiers.
+
 Wduplicated-branches
 C ObjC C++ ObjC++ Var(warn_duplicated_branches) Init(0) Warning
 Warn about duplicated branches in if-else statements.
===


2017-07-20  Volker Reichelt  <v.reich...@netcologne.de>

* cp-tree.h (struct saved_scope): New access_specifier_loc variable.
(current_access_specifier_loc): New macro.
* class.c (struct class_stack_node): New access_loc variable.
(pushclass): Push current_access_specifier_loc.  Set new
initial value.
(popclass): Pop current_access_specifier_loc.
* parser.c (cp_parser_member_specification_opt): Warn about
duplicated access-specifier.  Set current_access_specifier_loc.

Index: gcc/cp/cp-tree.h
===
--- gcc/cp/cp-tree.h(revision 250356)
+++ gcc/cp/cp-tree.h(working copy)
@@ -1531,6 +1531,7 @@
   tree class_name;
   tree class_type;
   tree access_specifier;
+  source_location access_specifier_loc;
   tree function_decl;
   vec<tree, va_gc> *lang_base;
   tree lang_name;
@@ -1592,6 +1593,7 @@
class, or union).  */
 
 #define current_access_specifier scope_chain->access_specifier
+#define current_access_specifier_loc scope_chain->access_specifier_loc
 
 /* Pointer to the top of the language name stack.  */
 
Index: gcc/cp/class.c
===
--- gcc/cp/class.c  (revision 250356)
+++ gcc/cp/class.c  (working copy)
@@ -60,6 +60,7 @@
   /* The access specifier pending for new declarations in the scope of
  this class.  */
   tree access;
+  location_t access

[PATCH] Remove redundant semicolons from libstdc++ headers

[Volker Reichelt]

Hi,

the following patch removes redundant semicolons after in-class
member function definitions from libstdc++-v3 headers.
These semicolons trigger warnings if you compile the respective
headers with "-Wextra-semi -Wsystem-headers".

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-07-19  Volker Reichelt  <v.reich...@netcologne.de>

* libsupc++/new (bad_array_new_length): Remove redundant
semicolon after in-class member function definition.
* include/bits/locale_facets.h (ctype_byname, num_put): Likewise.
* include/bits/locale_facets_nonio.h (time_put_byname): Likewise.
* include/bits/random.h (mersenne_twister_engine): Likewise.
* include/tr1/random.h (mersenne_twister): Likewise
* include/ext/random (simd_fast_mersenne_twister_engine): Likewise.
* include/ext/rope (char_producer, _Rope_char_consumer,
_Rope_self_destruct_ptr, _Rope_const_iterator, _Rope_iterator):
Likewise.
* include/ext/ropeimpl.h (_Rope_flatten_char_consumer,
_Rope_insert_char_consumer): Likewise.

Index: libstdc++-v3/libsupc++/new
===
--- libstdc++-v3/libsupc++/new  (revision 250281)
+++ libstdc++-v3/libsupc++/new  (working copy)
@@ -68,7 +68,7 @@
   class bad_array_new_length : public bad_alloc
   {
   public:
-bad_array_new_length() throw() { };
+bad_array_new_length() throw() { }
 
 // This declaration is not useless:
 // http://gcc.gnu.org/onlinedocs/gcc-3.0.2/gcc_6.html#SEC118
Index: libstdc++-v3/include/bits/locale_facets.h
===
--- libstdc++-v3/include/bits/locale_facets.h   (revision 250281)
+++ libstdc++-v3/include/bits/locale_facets.h   (working copy)
@@ -1487,7 +1487,7 @@
 
 protected:
   virtual
-  ~ctype_byname() { };
+  ~ctype_byname() { }
 };
 
   /// 22.2.1.4  Class ctype_byname specializations.
@@ -2486,7 +2486,7 @@
 
   /// Destructor.
   virtual
-  ~num_put() { };
+  ~num_put() { }
 
   //@{
   /**
Index: libstdc++-v3/include/bits/locale_facets_nonio.h
===
--- libstdc++-v3/include/bits/locale_facets_nonio.h (revision 250281)
+++ libstdc++-v3/include/bits/locale_facets_nonio.h (working copy)
@@ -898,7 +898,7 @@
   explicit
   time_put_byname(const char*, size_t __refs = 0)
   : time_put<_CharT, _OutIter>(__refs)
-  { };
+  { }
 
 #if __cplusplus >= 201103L
   explicit
Index: libstdc++-v3/include/bits/random.h
===
--- libstdc++-v3/include/bits/random.h  (revision 250281)
+++ libstdc++-v3/include/bits/random.h  (working copy)
@@ -520,7 +520,7 @@
*/
   static constexpr result_type
   min()
-  { return 0; };
+  { return 0; }
 
   /**
* @brief Gets the largest possible value in the output range.
Index: libstdc++-v3/include/tr1/random.h
===
--- libstdc++-v3/include/tr1/random.h   (revision 250281)
+++ libstdc++-v3/include/tr1/random.h   (working copy)
@@ -594,7 +594,7 @@
 
   result_type
   min() const
-  { return 0; };
+  { return 0; }
 
   result_type
   max() const
Index: libstdc++-v3/include/ext/random
===
--- libstdc++-v3/include/ext/random (revision 250281)
+++ libstdc++-v3/include/ext/random (working copy)
@@ -112,7 +112,7 @@
 
   static constexpr result_type
   min()
-  { return 0; };
+  { return 0; }
 
   static constexpr result_type
   max()
Index: libstdc++-v3/include/ext/rope
===
--- libstdc++-v3/include/ext/rope   (revision 250281)
+++ libstdc++-v3/include/ext/rope   (working copy)
@@ -150,7 +150,7 @@
 class char_producer
 {
 public:
-  virtual ~char_producer() { };
+  virtual ~char_producer() { }
 
   virtual void
   operator()(size_t __start_pos, size_t __len,
@@ -314,7 +314,7 @@
   // compile-time would do.  Hence this should all be private
   // for now.
   // The symmetry with char_producer is accidental and temporary.
-  virtual ~_Rope_char_consumer() { };
+  virtual ~_Rope_char_consumer() { }
   
   virtual bool
   operator()(const _CharT* __buffer, size_t __len) = 0;
@@ -924,9 +924,9 @@
   ~_Rope_self_destruct_ptr()
   { _Rope_RopeRep<_CharT, _Alloc>::_S_unref(_M_ptr); }
 #if __cpp_exceptions
-  _Rope_self_destruct_ptr() : _M_ptr(0) { };
+  _Rope_self_destruct_ptr() : _M_ptr(0) { }
 #else
-  _Rope_self_destruct_ptr() { };
+  _Rope_self_destruct_ptr() { }
 #endif
   _Rope_self_destruct_ptr(_Rope_RopeRep<_CharT, _Alloc>* __p

Re: [PING^4] {C++ PATCH] Fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

On 17 Jul, David Malcolm wrote:
> On Sun, 2017-07-16 at 12:13 +0200, Volker Reichelt wrote:
>> Hi,
>> 
>> this is PING^4 for a C++ patch that adds fix-it hints for wrong usage
>> of 'friend' and 'auto':
>> 
>> https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01575.html
> 
> I notice that the patch slightly changes the apparent indentation
> within the RID_AUTO hunk, but the pre-existing lines there are a
> mixture of pure spaces, and tabs+spaces; please double-check that any
> lines that the patch touches are converted to correctly follow our
> convention of tabs+spaces.

The indentation in that area needs quite some tabs vs. spaces cleanup.
I only fixed the lines that are touched by the code changes (and also
the comment above in the final commit).

> Other than that, OK for trunk.

Committed to trunk.

Regards,
VOlker

> Sorry for not spotting this earlier.
> Dave
> 
>> Previous pings
>> 
>> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01248.html
>> https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00207.html
>> https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00712.html ; (by
>> Gerald)
>> 
>> (thanks Gerald!)
>> 
>> Regards,
>> Volker
> 

[PING^4] {C++ PATCH] Fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

Hi,

this is PING^4 for a C++ patch that adds fix-it hints for wrong usage
of 'friend' and 'auto':

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01575.html

Previous pings:

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01248.html
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00207.html
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00712.html  (by Gerald)

(thanks Gerald!)

Regards,
Volker

Re: [PING] C++ Re: [PATCH] C/C++: fix quoting of "aka" typedef information (PR 62170)

[Volker Reichelt]

On 14 Jul, Martin Sebor wrote:
> On 06/21/2017 01:59 AM, Volker Reichelt wrote:
>> On 20 Jun, Jason Merrill wrote:
>>> On Tue, Jun 20, 2017 at 3:06 PM, David Malcolm <dmalc...@redhat.com> wrote:
>>>> It's not clear to me what the issue alluded to with negative
>>>> obstack_blank is, but I chose to follow the above docs and use
>>>> obstack_blank_fast; am testing an updated patch in which the above line
>>>> now looks like:
>>>>
>>>>   obstack_blank_fast (ob, -(type_start + type_len));
>>>>
>>>> Is the patch OK with that change? (assuming bootstrap
>>>> pass), or should I re-post?
>>>
>>> OK with that change.
>>>
>>>> On a related matter, this patch conflicts with Volker's patch here:
>>>>
>>>>   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01576.html
>>>>
>>>> in which he removes the trailing "{enum}" info (and hence all of our
>>>> changes to the testsuite conflict between the two patches...)
>>>>
>>>> Do you have any thoughts on that other patch? [Ccing Volker]
>>>
>>> That patch makes sense to me; I prefer "enum E" to "E {enum}".
>>>
>>> Jason
>>
>> Is 'makes sense' equivalent to 'OK for trunk' here? If so, should my
>> patch go in before David's or should we do it the other way round?
> 
> I missed this and have been pinging your patch on your behalf
> (below).  In the interest on making progress on this, IMO trivial,
> change I recommend taking Jason's comment as approval.
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00472.html
> 
> Martin

After several pings

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01248.html
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00207.html
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00712.html  (by Gerald)
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01487.html  (by Martin)
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00472.html  (by Martin)

(thanks Gerald and Martin!) for

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01576.html

I went with Martin's suggestion and took Jason's 'make sense' as an OK
for this trivial patch. Due to changes of an error message and a
testcase the patch had to be updated slightly (attached below)

Bootstrapped and regtested again on x86_64-pc-linux-gnu.
Applied to trunk.

Regards,
Volker


2017-07-16  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_cast_expression): Use %q#T instead of %qT
in old-style cast diagnostic.
* typeck.c (maybe_warn_about_useless_cast): Use %q#T instead of %qT
in useless cast diagnostic.
* error.c (type_to_string): Remove enum special handling.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 250230)
+++ gcc/cp/parser.c (working copy)
@@ -8890,7 +8890,7 @@
  maybe_add_cast_fixit (_loc, open_paren_loc, 
close_paren_loc,
expr, type);
  warning_at_rich_loc (_loc, OPT_Wold_style_cast,
-  "use of old-style cast to %qT", type);
+  "use of old-style cast to %q#T", type);
}
 
  /* Only type conversions to integral or enumeration types
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 250230)
+++ gcc/cp/typeck.c (working copy)
@@ -6681,7 +6681,7 @@
   ? xvalue_p (expr) : lvalue_p (expr))
   && same_type_p (TREE_TYPE (expr), TREE_TYPE (type)))
  || same_type_p (TREE_TYPE (expr), type))
-   warning (OPT_Wuseless_cast, "useless cast to type %qT", type);
+   warning (OPT_Wuseless_cast, "useless cast to type %q#T", type);
 }
 }
 
Index: gcc/cp/error.c
===
--- gcc/cp/error.c  (revision 250230)
+++ gcc/cp/error.c  (working copy)
@@ -3172,10 +3172,6 @@
   if (len == aka_len && memcmp (p, p+aka_start, len) == 0)
p[len] = '\0';
 }
-
-  if (typ && TYPE_P (typ) && TREE_CODE (typ) == ENUMERAL_TYPE)
-pp_string (cxx_pp, M_(" {enum}"));
-
   return pp_ggc_formatted_text (cxx_pp);
 }
 
===

2017-07-16  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/cpp1z/direct-enum-init1.C: Revert special enum handling.
* g++.dg/warn/pr12242.C: Likewise.

Index: gcc/testsuite/g++.dg/cpp1z/direct-enum-init1.C
===
--- gcc/te

Re: [PING] C++ Re: [PATCH] C/C++: fix quoting of "aka" typedef information (PR 62170)

[Volker Reichelt]

On 20 Jun, Jason Merrill wrote:
> On Tue, Jun 20, 2017 at 3:06 PM, David Malcolm  wrote:
>> It's not clear to me what the issue alluded to with negative
>> obstack_blank is, but I chose to follow the above docs and use
>> obstack_blank_fast; am testing an updated patch in which the above line
>> now looks like:
>>
>>   obstack_blank_fast (ob, -(type_start + type_len));
>>
>> Is the patch OK with that change? (assuming bootstrap
>> pass), or should I re-post?
> 
> OK with that change.
> 
>> On a related matter, this patch conflicts with Volker's patch here:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01576.html
>>
>> in which he removes the trailing "{enum}" info (and hence all of our
>> changes to the testsuite conflict between the two patches...)
>>
>> Do you have any thoughts on that other patch? [Ccing Volker]
> 
> That patch makes sense to me; I prefer "enum E" to "E {enum}".
> 
> Jason

Is 'makes sense' equivalent to 'OK for trunk' here? If so, should my
patch go in before David's or should we do it the other way round?

Regards,
Volker

Re: [PATCH] Add missing entry for -Wduplicated-branches

[Volker Reichelt]

On  5 Jun, Marek Polacek wrote:
> On Mon, Jun 05, 2017 at 02:38:11PM +0200, Volker Reichelt wrote:
>> Hi,
>> 
>> the warning option -Wduplicated-branches added by Marek in r244705
>> https://gcc.gnu.org/viewcvs?rev=244705=gcc=rev
>> lacks an entry in the warning list of doc/invoke.texi.
> 
> Oops.
> 
>> The patch below fixes that.
>> 
>> OK for trunk?
>> What about the GCC 7 branch?
> 
> Can't approve, but I think this is obvious, so I'd just go ahead with this.

Seems reasonable.
Committed as obvious to trunk and GCC 7 branch.

Regards,
Volker

>> Regards,
>> Volker
>> 
>> 2017-06-05  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>>  * doc/invoke.texi (-Wduplicated-branches): Add to warning list.
>> 
>> Index: gcc/doc/invoke.texi
>> ===
>> --- gcc/doc/invoke.texi  (revision 248863)
>> +++ gcc/doc/invoke.texi  (working copy)
>> @@ -273,7 +273,8 @@
>>  -Wno-deprecated  -Wno-deprecated-declarations  -Wno-designated-init @gol
>>  -Wdisabled-optimization @gol
>>  -Wno-discarded-qualifiers  -Wno-discarded-array-qualifiers @gol
>> --Wno-div-by-zero  -Wdouble-promotion  -Wduplicated-cond @gol
>> +-Wno-div-by-zero  -Wdouble-promotion @gol
>> +-Wduplicated-branches  -Wduplicated-cond @gol
>>  -Wempty-body  -Wenum-compare  -Wno-endif-labels  -Wexpansion-to-defined @gol
>>  -Werror  -Werror=*  -Wextra-semi  -Wfatal-errors @gol
>>  -Wfloat-equal  -Wformat  -Wformat=2 @gol
>> ===
>> 
> 
>   Marek

[PATCH] Add missing entry for -Wduplicated-branches

[Volker Reichelt]

Hi,

the warning option -Wduplicated-branches added by Marek in r244705
https://gcc.gnu.org/viewcvs?rev=244705=gcc=rev
lacks an entry in the warning list of doc/invoke.texi.
The patch below fixes that.

OK for trunk?
What about the GCC 7 branch?

Regards,
Volker

2017-06-05  Volker Reichelt  <v.reich...@netcologne.de>

* doc/invoke.texi (-Wduplicated-branches): Add to warning list.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 248863)
+++ gcc/doc/invoke.texi (working copy)
@@ -273,7 +273,8 @@
 -Wno-deprecated  -Wno-deprecated-declarations  -Wno-designated-init @gol
 -Wdisabled-optimization @gol
 -Wno-discarded-qualifiers  -Wno-discarded-array-qualifiers @gol
--Wno-div-by-zero  -Wdouble-promotion  -Wduplicated-cond @gol
+-Wno-div-by-zero  -Wdouble-promotion @gol
+-Wduplicated-branches  -Wduplicated-cond @gol
 -Wempty-body  -Wenum-compare  -Wno-endif-labels  -Wexpansion-to-defined @gol
 -Werror  -Werror=*  -Wextra-semi  -Wfatal-errors @gol
 -Wfloat-equal  -Wformat  -Wformat=2 @gol
===

Re: {PATCH] New C++ warning -Wcatch-value

[Volker Reichelt]

On 15 May, Martin Sebor wrote:
>> So how about the following then? I stayed with the catch part and added
>> a parameter to the warning to let the user decide on the warnings she/he
>> wants to get: -Wcatch-value=n.
>> -Wcatch-value=1 only warns for polymorphic classes that are caught by
>> value (to avoid slicing), -Wcatch-value=2 warns for all classes that
>> are caught by value (to avoid copies). And finally -Wcatch-value=3
>> warns for everything not caught by reference to find typos (like pointer
>> instead of reference) and bad coding practices.
> 
> It seems reasonable to me.  I'm not too fond of multi-level
> warnings since few users take advantage of anything but the
> default, but this case is simple and innocuous enough that
> I don't think it can do harm.
> 
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?
>>
>> If so, would it make sense to add -Wcatch-value=1 to -Wextra or even -Wall?
>> I would do this in a seperate patch, becuase I haven't checked what that
>> would mean for the testsuite.
> 
> I can't think of a use case for polymorphic slicing that's not
> harmful so unless there is a common one that escapes me, I'd say
> -Wall.

So that's what I committed after Jason's OK.

> What are your thoughts on enhancing the warning to also handle
> the rethrow case?
> 
> Also, it seems that a similar warning would be useful even beyond
> catch handlers, to help detect slicing when passing arguments to
> functions by value.  Especially in code that mixes OOP with the
> STL (or other template libraries).  Have you thought about tackling
> that at some point as well?

I don't have any plans to handle handle the rethrow case.

A general slicing warning would be very nice to have. Actually
clang-tidy has one (which is a little buggy, though).
Implementing this is over my head, though.
I'd rather stick with something less ambitious.

> Martin
> 
>>
>> Regards,
>> Volker

[PING^2] for three diagnostic-related patches

[Volker Reichelt]

Hi again,

I'd like to re-ping those patches below before they
start to bit-rot ;-)

Regards,
Volker

On 16 May, Volker Reichelt wrote:
> Hi,
> 
> I'd like to ping the following diagnostic-related patches that might
> have slipped through the cracks:
> 
> C++ parser: Fix typos in error messages:
> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00048.html
> 
> Fix-it hints for wrong usage of 'friend' and 'auto':
> https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01575.html
> 
> And finally, a follow-up patch:
> https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01576.html
> 
> Regards,
> Volker

Re: {PATCH] New C++ warning -Wcatch-value

[Volker Reichelt]

On 24 May, Jason Merrill wrote:
> On Mon, May 15, 2017 at 3:58 PM, Martin Sebor <mse...@gmail.com> wrote:
>>> So how about the following then? I stayed with the catch part and added
>>> a parameter to the warning to let the user decide on the warnings she/he
>>> wants to get: -Wcatch-value=n.
>>> -Wcatch-value=1 only warns for polymorphic classes that are caught by
>>> value (to avoid slicing), -Wcatch-value=2 warns for all classes that
>>> are caught by value (to avoid copies). And finally -Wcatch-value=3
>>> warns for everything not caught by reference to find typos (like pointer
>>> instead of reference) and bad coding practices.
>>
>> It seems reasonable to me.  I'm not too fond of multi-level
>> warnings since few users take advantage of anything but the
>> default, but this case is simple and innocuous enough that
>> I don't think it can do harm.
> 
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>> OK for trunk?
> 
> OK.

Committed.

>>> If so, would it make sense to add -Wcatch-value=1 to -Wextra or even -Wall?
>>> I would do this in a seperate patch, becuase I haven't checked what that
>>> would mean for the testsuite.
>>
>> I can't think of a use case for polymorphic slicing that's not
>> harmful so unless there is a common one that escapes me, I'd say
>> -Wall.
> 
> Agreed.  But then you'll probably want to allow -Wno-catch-value to turn it 
> off.
> 
> Jason

So how about the following then?
Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker

2017-05-30  Volker Reichelt  <v.reich...@netcologne.de>

* doc/invoke.texi (-Wcatch-value): Document new shortcut.
Add to -Wall section.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 248585)
+++ gcc/doc/invoke.texi (working copy)
@@ -265,8 +265,8 @@
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  @gol
--Wchar-subscripts  -Wchkp  -Wcatch-value=@var{n}  -Wclobbered  -Wcomment  @gol
--Wconditionally-supported  @gol
+-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
+-Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
 -Wdelete-incomplete @gol
 -Wno-deprecated  -Wno-deprecated-declarations  -Wno-designated-init @gol
@@ -3757,6 +3757,7 @@
 -Wbool-compare  @gol
 -Wbool-operation  @gol
 -Wc++11-compat  -Wc++14-compat  @gol
+-Wcatch-value @r{(C++ and Objective-C++ only)}  @gol
 -Wchar-subscripts  @gol
 -Wcomment  @gol
 -Wduplicate-decl-specifier @r{(C and Objective-C only)} @gol
@@ -5834,13 +5835,16 @@
 literals to @code{char *}.  This warning is enabled by default for C++
 programs.
 
-@item -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)}
+@item -Wcatch-value
+@itemx -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)}
 @opindex Wcatch-value
+@opindex Wno-catch-value
 Warn about catch handlers that do not catch via reference.
-With @option{-Wcatch-value=1} warn about polymorphic class types that
-are caught by value. With @option{-Wcatch-value=2} warn about all class
-types that are caught by value. With @option{-Wcatch-value=3} warn about
-all types that are not caught by reference.
+With @option{-Wcatch-value=1} (or @option{-Wcatch-value} for short)
+warn about polymorphic class types that are caught by value.
+With @option{-Wcatch-value=2} warn about all class types that are caught
+by value. With @option{-Wcatch-value=3} warn about all types that are
+not caught by reference. @option{-Wcatch-value} is enabled by @option{-Wall}.
 
 @item -Wclobbered
 @opindex Wclobbered
===

2017-05-30  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Wcatch-value): New shortcut for Wcatch-value=1.
(Wcatch-value=1): Enable by -Wall.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 248585)
+++ gcc/c-family/c.opt  (working copy)
@@ -388,8 +388,12 @@
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
 
+Wcatch-value
+C++ ObjC++ Warning Alias(Wcatch-value=, 1, 0)
+Warn about catch handlers of non-reference type.
+
 Wcatch-value=
-C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger
+C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger 
LangEnabledBy(C++ ObjC++,Wall, 1, 0)
 Warn about catch handlers of non-reference type.
 
 Wchar-subscripts
===

[PING] for three diagnostic-related patches

[Volker Reichelt]

Hi,

I'd like to ping the following diagnostic-related patches that might
have slipped through the cracks:

C++ parser: Fix typos in error messages:
https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00048.html

Fix-it hints for wrong usage of 'friend' and 'auto':
https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01575.html

And finally, a follow-up patch:
https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01576.html

Regards,
Volker

Re: {PATCH] New C++ warning -Wcatch-value

[Volker Reichelt]

On  7 May, Martin Sebor wrote:
> On 05/07/2017 02:03 PM, Volker Reichelt wrote:
>> On  2 May, Martin Sebor wrote:
>>> On 05/01/2017 02:38 AM, Volker Reichelt wrote:
>>>> Hi,
>>>>
>>>> catching exceptions by value is a bad thing, as it may cause slicing, i.e.
>>>> a) a superfluous copy
>>>> b) which is only partial.
>>>> See also 
>>>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference
>>>>
>>>> To warn the user about catch handlers of non-reference type,
>>>> the following patch adds a new C++/ObjC++ warning option "-Wcatch-value".
>>>
>>> I think the problems related to catching exceptions by value
>>> apply to (a subset of) class types but not so much to fundamental
>>> types.  I would expect indiscriminately warning on every type to
>>> be overly restrictive.
>>>
>>> The Enforcement section of the C++ guideline suggests to
>>>
>>>Flag by-value exceptions if their types are part of a hierarchy
>>>(could require whole-program analysis to be perfect).
>>>
>>> The corresponding CERT C++ Coding Standard guideline offers
>>> a similar suggestion here:
>>>
>>>https://www.securecoding.cert.org/confluence/x/TAD5CQ
>>>
>>> so I would suggest to model the warning on that approach (within
>>> limits of a single translation unit, of course).   I.e., warn only
>>> for catching by value objects of non-trivial types, or perhaps even
>>> only polymorphic types?
>>>
>>> Martin
>>
>> I've never seen anybody throw integers in real-world code, so I didn't
>> want to complicate things for this case. But maybe I should only warn
>> about class-types.
>>
>> IMHO it makes sense to warn about non-polymorphic class types
>> (although slicing is not a problem there), because you still have to
>> deal with redundant copies.
>>
>> Another thing would be pointers. I've never seen pointers in catch
>> handlers (except some 'catch (const char*)' which I would consider
>> bad practice). Therefore I'd like to warn about 'catch (const A*)'
>> which might be a typo that should read 'catch (const A&)' instead.
>>
>> Would that be OK?
> 
> To my knowledge, catch by value of non-polymorphic types (and
> certainly fundamental types) is not a cause of common bugs.
> It's part of the recommended practice to throw by value, catch
> by reference, which is grounded in avoiding the slicing problem.
> It's also sometimes recommended for non-trivial class types to
> avoid creating a copy of the object (which, for non-trivial types,
> may need to allocate resource and could throw).  Otherwise, it's
> not dissimilar to pass-by value vs pass-by-reference (or even
> pass-by-pointer).  Both may be good practices for some types or
> in some situations but neither is necessary to avoid bugs or
> universally applicable to achieve superior performance.
> 
> The pointer case is interesting.  In C++ Coding Standards,
> Sutter and Alexandrescu recommend to throw (and catch) smart
> pointers over plain pointers because it obviates having to deal
> with memory management issues.  That's sound advice but it seems
> more like a design guideline than a coding rule aimed at directly
> preventing bugs.  I also think that the memory management bugs
> that it might find might be more easily detected at the throw
> site instead.  E.g., warning on the throw expression below:
> 
>{
>  Exception e;
>  throw 
>}
> 
> or perhaps even on
> 
>{
>  throw *new Exception ();
>}
> 
> A more sophisticated (and less restrictive) checker could detect
> and warn on "throw <T*>" if it found a catch (T) or catch (T&)
> in the same file and no catch (T*) (but not warn otherwise).
> 
> Martin
> 
> PS After re-reading some of the coding guidelines on this topic
> it occurs to me that (if your patch doesn't handle this case yet)
> it might be worth considering to enhance it to also warn on
> rethrowing caught polymorphic objects (i.e., warn on
> 
>catch (E ) { throw e; }
> 
> and suggest to use "throw;" instead, for the same reason: to
> help avoid slicing.
> 
> PPS  It may be a useful feature to implement some of other ideas
> you mentioned (e.g., throw by value rather than pointer) but it
> feels like a separate and more ambitious project than detecting
> the relatively common and narrow slicing problem.

So how about the following then? I stayed with

Re: [PATCH] Pretty-printing of some unsupported expressions (PR c/35441)

[Volker Reichelt]

On  2 May, Joseph Myers wrote:
> On Fri, 10 Mar 2017, Volker Reichelt wrote:
> 
>> a) This part (with foo1 and foo2 from the testcase) is straightforward.
> 
> That part is OK.
> 
>> b) I chose the shift operators 'a << b' and 'a >> b' for the rotate
>>expressions, which is not 100% correct. Would it be better to use
>>something like 'lrotate(a, b)', '__lrotate__(a, b)' or 'a lrotate b'
>>instead? Or is there something like an '__builtin_lrotate' that I misseed?
> 
> I'd be inclined to use the notation <<< and >>> for rotation, cf. 
> <https://stackoverflow.com/questions/32785998/symbol-for-bitwise-circular-shifts>.

Nice idea. It would be nice to see these operators in some future C/C++
versions. Is the nested ternary operator used in the updated patch below
OK, or would you prefer a switch?

>> c) I chose 'max(q, b)' and 'min(q, b)'.
> 
> I think that's fine.
> 
>> In addition I found some more division operators in gcc/tree.def that
>> aren't handled by the pretty-printer:
>> 
>>   CEIL_DIV_EXPR
>>   FLOOR_DIV_EXPR
>>   ROUND_DIV_EXPR
>>   CEIL_MOD_EXPR
>>   FLOOR_MOD_EXPR
>>   ROUND_MOD_EXPR
>> 
>> Alas I don't have testcases for them. Nevertheless, I could handle them
>> like the other MOD and DIV operators just to be safe.
> 
> These can probably appear from Ada code, but maybe not from C.

OK, I'll ignore them.

> Now we have caret diagnostics and location ranges I think we should be 
> moving away from printing complicated expressions from trees anyway.  So 
> for the diagnostics about calling non-functions, it would be best to make 
> a location range for the called expression available if it isn't already, 
> then do a diagnostic with a location that underlines that text rather than 
> trying to reproduce an expression text from trees.

Indeed, we can do better with caret diagnostics. But non-caret mode is
still there (and has its uses because of its usually more consise form)
and there are probably more places where this expression is printed.
Therefore, I'd like to fix this regardless of a better caret solution
for the diagnostics about calling non-functions.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-05-07  Volker Reichelt  <v.reich...@netcologne.de>

PR c/35441
* c-pretty-print.c (c_pretty_printer::expression): Handle MAX_EXPR,
MIN_EXPR, EXACT_DIV_EXPR, RDIV_EXPR, LROTATE_EXPR, RROTATE_EXPR.
(c_pretty_printer::postfix_expression): Handle MAX_EXPR, MIN_EXPR.
(c_pretty_printer::multiplicative_expression): Handle EXACT_DIV_EXPR,
RDIV_EXPR.
(pp_c_shift_expression): Handle LROTATE_EXPR, RROTATE_EXPR.

Index: gcc/c-family/c-pretty-print.c
===
--- gcc/c-family/c-pretty-print.c   (revision 247727)
+++ gcc/c-family/c-pretty-print.c   (working copy)
@@ -1551,6 +1551,14 @@
   : "__builtin_islessgreater");
   goto two_args_fun;
 
+case MAX_EXPR:
+  pp_c_ws_string (this, "max");
+  goto two_args_fun;
+
+case MIN_EXPR:
+  pp_c_ws_string (this, "min");
+  goto two_args_fun;
+
 two_args_fun:
   pp_c_left_paren (this);
   expression (TREE_OPERAND (e, 0));
@@ -1829,6 +1837,8 @@
 case MULT_EXPR:
 case TRUNC_DIV_EXPR:
 case TRUNC_MOD_EXPR:
+case EXACT_DIV_EXPR:
+case RDIV_EXPR:
   multiplicative_expression (TREE_OPERAND (e, 0));
   pp_c_whitespace (this);
   if (code == MULT_EXPR)
@@ -1890,9 +1900,13 @@
 {
 case LSHIFT_EXPR:
 case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
   pp_c_shift_expression (pp, TREE_OPERAND (e, 0));
   pp_c_whitespace (pp);
-  pp_string (pp, code == LSHIFT_EXPR ? "<<" : ">>");
+  pp_string (pp, code == LSHIFT_EXPR ? "<<" :
+code == RSHIFT_EXPR ? ">>" :
+code == LROTATE_EXPR ? "<<<" : ">>>");
   pp_c_whitespace (pp);
   pp_c_additive_expression (pp, TREE_OPERAND (e, 1));
   break;
@@ -2186,6 +2200,8 @@
 case UNLT_EXPR:
 case UNGE_EXPR:
 case UNGT_EXPR:
+case MAX_EXPR:
+case MIN_EXPR:
 case ABS_EXPR:
 case CONSTRUCTOR:
 case COMPOUND_LITERAL_EXPR:
@@ -2217,11 +2233,15 @@
 case MULT_EXPR:
 case TRUNC_MOD_EXPR:
 case TRUNC_DIV_EXPR:
+case EXACT_DIV_EXPR:
+case RDIV_EXPR:
   multiplicative_expression (e);
   break;
 
 case LSHIFT_EXPR:
 case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
   pp_c_shift_expression (this, e);
   break;
 
=

Re: {PATCH] New C++ warning -Wcatch-value

[Volker Reichelt]

On  2 May, Martin Sebor wrote:
> On 05/01/2017 02:38 AM, Volker Reichelt wrote:
>> Hi,
>>
>> catching exceptions by value is a bad thing, as it may cause slicing, i.e.
>> a) a superfluous copy
>> b) which is only partial.
>> See also 
>> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference
>>
>> To warn the user about catch handlers of non-reference type,
>> the following patch adds a new C++/ObjC++ warning option "-Wcatch-value".
> 
> I think the problems related to catching exceptions by value
> apply to (a subset of) class types but not so much to fundamental
> types.  I would expect indiscriminately warning on every type to
> be overly restrictive.
> 
> The Enforcement section of the C++ guideline suggests to
> 
>Flag by-value exceptions if their types are part of a hierarchy
>(could require whole-program analysis to be perfect).
> 
> The corresponding CERT C++ Coding Standard guideline offers
> a similar suggestion here:
> 
>https://www.securecoding.cert.org/confluence/x/TAD5CQ
> 
> so I would suggest to model the warning on that approach (within
> limits of a single translation unit, of course).   I.e., warn only
> for catching by value objects of non-trivial types, or perhaps even
> only polymorphic types?
> 
> Martin

I've never seen anybody throw integers in real-world code, so I didn't
want to complicate things for this case. But maybe I should only warn
about class-types.

IMHO it makes sense to warn about non-polymorphic class types
(although slicing is not a problem there), because you still have to
deal with redundant copies.

Another thing would be pointers. I've never seen pointers in catch
handlers (except some 'catch (const char*)' which I would consider
bad practice). Therefore I'd like to warn about 'catch (const A*)'
which might be a typo that should read 'catch (const A&)' instead.

Would that be OK?

Regards,
Volker

>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>
>>
>>  * doc/invoke.texi (-Wcatch-value): Document new warning option.
>>
>> Index: gcc/doc/invoke.texi
>> ===
>> --- gcc/doc/invoke.texi  (revision 247416)
>> +++ gcc/doc/invoke.texi  (working copy)
>> @@ -265,7 +265,7 @@
>>  -Wno-builtin-declaration-mismatch @gol
>>  -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
>>  -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  
>> @gol
>> --Wchar-subscripts -Wchkp  -Wclobbered  -Wcomment  @gol
>> +-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wclobbered  -Wcomment  @gol
>>  -Wconditionally-supported  @gol
>>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time 
>> @gol
>>  -Wdelete-incomplete @gol
>> @@ -5827,6 +5827,11 @@
>>  literals to @code{char *}.  This warning is enabled by default for C++
>>  programs.
>>
>> +@item -Wcatch-value @r{(C++ and Objective-C++ only)}
>> +@opindex Wcatch-value
>> +@opindex Wno-catch-value
>> +Warn about catch handler of non-reference type.
>> +
>>  @item -Wclobbered
>>  @opindex Wclobbered
>>  @opindex Wno-clobbered
>> ===
>>
>> 2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>
>>
>>  * c.opt (Wcatch-value): New C++ warning flag.
>>
>> Index: gcc/c-family/c.opt
>> ===
>> --- gcc/c-family/c.opt   (revision 247416)
>> +++ gcc/c-family/c.opt   (working copy)
>> @@ -388,6 +388,10 @@
>>  C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
>>  Warn about casts which discard qualifiers.
>>
>> +Wcatch-value
>> +C++ ObjC++ Var(warn_catch_value) Warning
>> +Warn about catch handlers of non-reference type.
>> +
>>  Wchar-subscripts
>>  C ObjC C++ ObjC++ Var(warn_char_subscripts) Warning LangEnabledBy(C ObjC 
>> C++ ObjC++,Wall)
>>  Warn about subscripts whose type is \"char\".
>> ===
>>
>> 2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>
>>
>>  * semantics.c (finish_handler_parms): Warn about non-reference type
>>  catch handlers.
>>
>> Index: gcc/cp/semantics.c
>> 

Re: [PATCH] PR80280: Fix quoting of candidate functions

[Volker Reichelt]

OK, committed as obvious (r247728).

Regards,
Volker

On  7 May, Martin Sebor wrote:
> On 05/06/2017 02:41 PM, Volker Reichelt wrote:
>> Hi,
>>
>> the following patch fixes some wrong quoting that was introduced by
>> Martin's patch for PR translation/80280, see
>> https://gcc.gnu.org/viewcvs/gcc?view=revision=247607
>>
>> Consider the following testcase:
>>  struct A {};
>>  bool b = !A();
>>
>> On trunk we currently get the following diagnostic:
>>  bug.cc:2:10: error: no match for 'operator!' (operand type is 'A')
>>   bool b = !A();
>>^~~~
>>  bug.cc:2:10: note: 'candidate: operator!(bool) '
>>  bug.cc:2:10: note:   no known conversion for argument 1 from 'A' to 'bool'
>>
>> Note, that not only the candidate function, but also the surrounding
>> text is quoted in the second-to-last line.
>>
>> With the patch, this line reads:
>>  bug.cc:2:10: note: candidate: 'operator!(bool)' 
> 
> This quoting looks better, thanks for the correction.  I would
> say it falls under the obvious fix category.
> 
> Incidentally, the candidate for the test case (and other Boolean
> expressions involving the struct) doesn't look very helpful or
> even relevant, but that's a separate issue.
> 
> Martin
> 
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-05-06  Volker Reichelt  <v.reich...@netcologne.de>
>>
>>  PR translation/80280
>>  * call.c (print_z_candidate): Fix quoting.
>>
>> Index: gcc/cp/call.c
>> ===
>> --- gcc/cp/call.c(revision 247720)
>> +++ gcc/cp/call.c(working copy)
>> @@ -3457,16 +3457,16 @@
>>  {
>>cloc = loc;
>>if (candidate->num_convs == 3)
>> -inform (cloc, "%<%s%D(%T, %T, %T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T, %T, %T)%> ", msg, fn,
>>  candidate->convs[0]->type,
>>  candidate->convs[1]->type,
>>  candidate->convs[2]->type);
>>else if (candidate->num_convs == 2)
>> -inform (cloc, "%<%s%D(%T, %T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T, %T)%> ", msg, fn,
>>  candidate->convs[0]->type,
>>  candidate->convs[1]->type);
>>else
>> -inform (cloc, "%<%s%D(%T) %>", msg, fn,
>> +inform (cloc, "%s%<%D(%T)%> ", msg, fn,
>>  candidate->convs[0]->type);
>>  }
>>else if (TYPE_P (fn))
>> ===

[PATCH] PR80280: Fix quoting of candidate functions

[Volker Reichelt]

Hi,

the following patch fixes some wrong quoting that was introduced by
Martin's patch for PR translation/80280, see
https://gcc.gnu.org/viewcvs/gcc?view=revision=247607

Consider the following testcase:
 struct A {};
 bool b = !A();

On trunk we currently get the following diagnostic:
 bug.cc:2:10: error: no match for 'operator!' (operand type is 'A')
  bool b = !A();
   ^~~~
 bug.cc:2:10: note: 'candidate: operator!(bool) '
 bug.cc:2:10: note:   no known conversion for argument 1 from 'A' to 'bool'

Note, that not only the candidate function, but also the surrounding
text is quoted in the second-to-last line.

With the patch, this line reads:
 bug.cc:2:10: note: candidate: 'operator!(bool)' 

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-05-06  Volker Reichelt  <v.reich...@netcologne.de>

PR translation/80280
* call.c (print_z_candidate): Fix quoting.

Index: gcc/cp/call.c
===
--- gcc/cp/call.c   (revision 247720)
+++ gcc/cp/call.c   (working copy)
@@ -3457,16 +3457,16 @@
 {
   cloc = loc;
   if (candidate->num_convs == 3)
-   inform (cloc, "%<%s%D(%T, %T, %T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T, %T, %T)%> ", msg, fn,
candidate->convs[0]->type,
candidate->convs[1]->type,
candidate->convs[2]->type);
   else if (candidate->num_convs == 2)
-   inform (cloc, "%<%s%D(%T, %T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T, %T)%> ", msg, fn,
candidate->convs[0]->type,
candidate->convs[1]->type);
   else
-   inform (cloc, "%<%s%D(%T) %>", msg, fn,
+   inform (cloc, "%s%<%D(%T)%> ", msg, fn,
candidate->convs[0]->type);
 }
   else if (TYPE_P (fn))
===

Re: [PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

On  2 May, Martin Sebor wrote:
> On 04/29/2017 04:23 PM, Volker Reichelt wrote:
>> Hi,
>>
>> the following patch adds fix-it hints to the C++ parser for two wrongly
>> used keywords detected in cp_parser_decl_specifier_seq.
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-04-29  Volker Reichelt  <v.reich...@netcologne.de>
>>
>>  * parser.c (cp_parser_decl_specifier_seq): Add fix-it hints for
>>  friend outside class and obsolete auto as storage-class-specifier.
>>
>> Index: gcc/cp/parser.c
>> ===
>> --- gcc/cp/parser.c  2017-04-28
>> +++ gcc/cp/parser.c  2017-04-28
>> @@ -13213,7 +13213,9 @@
>>  case RID_FRIEND:
>>if (!at_class_scope_p ())
>>  {
>> -  error_at (token->location, "%<friend%> used outside of class");
>> +  gcc_rich_location richloc (token->location);
>> +  richloc.add_fixit_remove ();
>> +  error_at_rich_loc (, "%<friend%> used outside of class");
>>cp_lexer_purge_token (parser->lexer);
>>  }
>>else
>> @@ -13277,8 +13279,11 @@
>>
>>/* Complain about `auto' as a storage specifier, if
>>   we're complaining about C++0x compatibility.  */
>> -  warning_at (token->location, OPT_Wc__11_compat, "%<auto%>"
>> -  " changes meaning in C++11; please remove it");
>> +  gcc_rich_location richloc (token->location);
>> +  richloc.add_fixit_remove ();
>> +  warning_at_rich_loc (, OPT_Wc__11_compat,
>> +   "%<auto%> changes meaning in C++11; "
>> +   "please remove it");
> 
> What caught my eye here is the "please remove it" part :)  Maybe
> it's me but it seems a little too forceful for a warning (despite
> the please).  I would find a more conventional phrasing like
> "suggest to remove it" to be more suitable.
> 
> That said, I wonder if removing the auto is actually the best
> suggestion.  Wouldn't it more in line with where C++ is headed
> to suggest to remove the type and keep the auto?
>
> Martin

I think you missed that we are in C++98 mode here. Dropping the 'auto'
is the only viable choice to stay in C++98 mode and gain C++11
compatibility.

With that in mind, I don't think that the wording "please remove it"
is bad. The user is in C++98 mode and wants to know about C++11
compatibility (as OPT_Wc__11_compat is selected). So the compiler
gives her/him clear advice what to do.
For me "suggest to remove it" sounde too weak (more like "if you run
into problems in C++11, you could try to drop the 'auto' here").
Other solutions like "'auto' needs to be removed here to gain C++11
compatibility" sound a bit clumsy to me.

Regards,
Volker

[PATCH] C++ parser: Fix typos in error messages

[Volker Reichelt]

Hi,

the following patch fixes two typos in error messages of the C++ parser
(which have gone unnoticed since GCC 3.4.0).

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?
Should this go also to GCC 7.2?

Regards,
Volker


2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_base_specifier): Fix typos in error messages.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247445)
+++ gcc/cp/parser.c (working copy)
@@ -23716,7 +23716,7 @@
  if (virtual_p && !duplicate_virtual_error_issued_p)
{
  cp_parser_error (parser,
-  "%<virtual%> specified more than once in 
base-specified");
+  "%<virtual%> specified more than once in 
base-specifier");
  duplicate_virtual_error_issued_p = true;
}
 
@@ -23736,7 +23736,7 @@
  && !duplicate_access_error_issued_p)
{
  cp_parser_error (parser,
-  "more than one access specifier in 
base-specified");
+  "more than one access specifier in 
base-specifier");
  duplicate_access_error_issued_p = true;
}
 
===

{PATCH] New C++ warning -Wcatch-value

[Volker Reichelt]

Hi,

catching exceptions by value is a bad thing, as it may cause slicing, i.e.
a) a superfluous copy
b) which is only partial.
See also 
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference

To warn the user about catch handlers of non-reference type,
the following patch adds a new C++/ObjC++ warning option "-Wcatch-value".

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>

* doc/invoke.texi (-Wcatch-value): Document new warning option.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi (revision 247416)
+++ gcc/doc/invoke.texi (working copy)
@@ -265,7 +265,7 @@
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  @gol
--Wchar-subscripts -Wchkp  -Wclobbered  -Wcomment  @gol
+-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wclobbered  -Wcomment  @gol
 -Wconditionally-supported  @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
 -Wdelete-incomplete @gol
@@ -5827,6 +5827,11 @@
 literals to @code{char *}.  This warning is enabled by default for C++
 programs.
 
+@item -Wcatch-value @r{(C++ and Objective-C++ only)}
+@opindex Wcatch-value
+@opindex Wno-catch-value
+Warn about catch handler of non-reference type.
+
 @item -Wclobbered
 @opindex Wclobbered
 @opindex Wno-clobbered
===

2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Wcatch-value): New C++ warning flag.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 247416)
+++ gcc/c-family/c.opt  (working copy)
@@ -388,6 +388,10 @@
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
 
+Wcatch-value
+C++ ObjC++ Var(warn_catch_value) Warning
+Warn about catch handlers of non-reference type.
+
 Wchar-subscripts
 C ObjC C++ ObjC++ Var(warn_char_subscripts) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wall)
 Warn about subscripts whose type is \"char\".
=======

2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>

* semantics.c (finish_handler_parms): Warn about non-reference type
catch handlers.

Index: gcc/cp/semantics.c
===
--- gcc/cp/semantics.c  (revision 247416)
+++ gcc/cp/semantics.c  (working copy)
@@ -1321,7 +1321,15 @@
}
 }
   else
-type = expand_start_catch_block (decl);
+{
+  type = expand_start_catch_block (decl);
+  if (warn_catch_value
+ && type != NULL_TREE
+ && type != error_mark_node
+ && TREE_CODE (TREE_TYPE (decl)) != REFERENCE_TYPE)
+   warning (OPT_Wcatch_value,
+"catching non-reference type %qT", TREE_TYPE (decl));
+}
   HANDLER_TYPE (handler) = type;
 }
 
=======
 
2017-05-01  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/warn/Wcatch-value-1.C: New test.

Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
===
--- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C  2017-05-01
+++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C  2017-05-01
@@ -0,0 +1,45 @@
+// { dg-options "-Wcatch-value" }
+
+struct A {};
+struct B {};
+struct C {};
+
+void foo()
+{
+  try {}
+  catch (int)  {}  // { dg-warning "catching non-reference type" }
+  catch (double*)  {}  // { dg-warning "catching non-reference type" }
+  catch (float&)   {}
+  catch (A){}  // { dg-warning "catching non-reference type" }
+  catch (A[2]) {}  // { dg-warning "catching non-reference type" }
+  catch (B*)   {}  // { dg-warning "catching non-reference type" }
+  catch (B&)   {}
+  catch (const C&) {}
+}
+
+template void foo1()
+{
+  try {}
+  catch (T) {}
+}
+
+void bar1()
+{
+  foo1<int&>();
+  foo1();
+}
+
+template void foo2()
+{
+  try {}
+  catch (T) {}  // { dg-warning "catching non-reference type" }
+
+  try {}
+  catch (T&) {}
+}
+
+void bar2()
+{
+  foo2<int*>();  // { dg-message "required" }
+  foo2(); // { dg-message "required" }
+}
===

Re: [PATCH] Improved diagnostics for casts and enums

[Volker Reichelt]

On 28 Apr, Volker Reichelt wrote:
> On 27 Apr, Martin Sebor wrote:
>> On 04/27/2017 01:29 AM, Volker Reichelt wrote:
>>> Hi,
>>>
>>> the following two patches aim at improving GCC's diagnostics to help
>>> the user to get rid of old-style casts. While old-style pointer casts
>>> are really bad and need to be weeded out quickly, old-style casts between
>>> arithmetic types are IMHO much more tolerable. The patches allow to
>>> easily distinguish between those situations.
>> 
>> FWIW, it can be most helpful to include this sort of detail (and
>> similar) in diagnostics.  In the case of the C-style cast, besides
>> mentioning the type of the result, it might be even more helpful
>> to mention the type of the operand because unlike that of the
>> result, its type is not apparent from the cast itself.
> 
> In this particular case the operand is evaluated after the warning
> message. So I couldn't include it in the message without fiddling
> around with the logic. I was also afraid that the warning message would
> become too long.
> 
>>> The first patch for cp_parser_cast_expression in parser.c just adds
>>> the target type of the cast to the diagnostic (like in
>>> maybe_warn_about_useless_cast in typeck.c).
>>>
>>> The second patch for type_to_string in error.c tackles the problem
>>> that the name of a type doesn't tell you if you have a class or just
>>> a simple enum. Similar to adding "{aka 'int'}" to types that
>>> are essentially integers, this patch adds "{enum}" to all
>>> enumeration types (and adjusts two testcases accordingly).
>> 
>> In the C front end %qT prints 'enum E' for an argument of
>> an enumerated type.  Is there some significance to having
>> the C++ front end print 'E { enum }' or can C++ be made
>> consistent?
>>
>> Martin
> 
> Ah, good point! I was so focused on the '{aka ...}' thing that I didn't
> see the obvious: I should have used %q#T instead of just %qT to get
> the enum prefix with the type. Unfortunately, I already committed the
> patch after Nathan's OK before seeing your message.
> 
> I'll prepare another patch this weekend to revert my error.c changes and
> use %q#T for the warnings about old-style and useless casts mentioned
> above.
> 
> Another thought just crossed my mind: Other users might find an
> enum/class prefix helpful for different warnings. Right now they
> would have to modify the compile to use %q#T instead of %qT.
> Would it make sense to add a compiler option that sets the verbose
> flag for cp_printer unconditionally? Or am I just unaware of existing
> functionality?
> 
> Regards,
> Volker

So here's the patch that reverts the special enum handling in 
type_to_string and uses %q#T instead of %qT for two casting-related
diagnostics.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

As one can see from the testsuite changes, there are several
casting- and conversion-related messages like "invalid conversion from",
"cannot convert", "invalid cast" that still use the simple %qT
form. I'll give it a try to use %q#T there as well and prepare a
separate patch if this really improves the diagnostics.

Regards,
Volker


2017-04-30  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_cast_expression): Use %q#T instead of %qT
in old-style cast diagnostic.
* typeck.c (maybe_warn_about_useless_cast): Use %q#T instead of %qT
in useless cast diagnostic.
* error.c (type_to_string): Remove enum special handling.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247394)
+++ gcc/cp/parser.c (working copy)
@@ -8764,7 +8764,7 @@
  && !VOID_TYPE_P (type)
  && current_lang_name != lang_name_c)
warning (OPT_Wold_style_cast,
-"use of old-style cast to %qT", type);
+"use of old-style cast to %q#T", type);
 
  /* Only type conversions to integral or enumeration types
 can be used in constant-expressions.  */
Index: gcc/cp/typeck.c
===
--- gcc/cp/typeck.c (revision 247394)
+++ gcc/cp/typeck.c (working copy)
@@ -6631,7 +6631,7 @@
   ? xvalue_p (expr) : lvalue_p (expr))
   && same_type_p (TREE_TYPE (expr), TREE_TYPE (type)))
  || same_type_p (TREE_TYPE (expr), type))
-   warning (OPT_Wuseless_cast, "useless cast to type %qT", type);
+   warning (OPT_Wuseless_cast, "useless cast t

[PATCH] c++ parser: fix-it hints for wrong usage of 'friend' and 'auto'

[Volker Reichelt]

Hi,

the following patch adds fix-it hints to the C++ parser for two wrongly
used keywords detected in cp_parser_decl_specifier_seq.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-04-29  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_decl_specifier_seq): Add fix-it hints for
friend outside class and obsolete auto as storage-class-specifier.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c 2017-04-28
+++ gcc/cp/parser.c 2017-04-28
@@ -13213,7 +13213,9 @@
case RID_FRIEND:
  if (!at_class_scope_p ())
{
- error_at (token->location, "%<friend%> used outside of class");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_remove ();
+ error_at_rich_loc (, "%<friend%> used outside of class");
  cp_lexer_purge_token (parser->lexer);
}
  else
@@ -13277,8 +13279,11 @@
 
   /* Complain about `auto' as a storage specifier, if
  we're complaining about C++0x compatibility.  */
-  warning_at (token->location, OPT_Wc__11_compat, "%<auto%>"
- " changes meaning in C++11; please remove it");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_remove ();
+ warning_at_rich_loc (, OPT_Wc__11_compat,
+  "%<auto%> changes meaning in C++11; "
+  "please remove it");
 
   /* Set the storage class anyway.  */
   cp_parser_set_storage_class (parser, decl_specs, RID_AUTO,
===

2017-04-29  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/diagnostic/friend1.C: New test.
* g++.dg/cpp0x/auto1.C: Add check for fix-it hint.

Index: gcc/testsuite/g++.dg/diagnostic/friend1.C
===
--- gcc/testsuite/g++.dg/diagnostic/friend1.C   2017-04-29
+++ gcc/testsuite/g++.dg/diagnostic/friend1.C   2017-04-29
@@ -0,0 +1,8 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+friend void foo();  /* { dg-error "used outside of class" }
+  { dg-begin-multiline-output "" }
+ friend void foo();
+ ^~
+ --
+  { dg-end-multiline-output "" } */
Index: gcc/testsuite/g++.dg/cpp0x/auto1.C
===
--- gcc/testsuite/g++.dg/cpp0x/auto1.C  (revision 247406)
+++ gcc/testsuite/g++.dg/cpp0x/auto1.C  (working copy)
@@ -1,9 +1,14 @@
 // { dg-do compile { target c++11 } }
-// { dg-options "-std=c++98 -Wc++11-compat" }
+// { dg-options "-std=c++98 -Wc++11-compat -fdiagnostics-show-caret" }
 
 // Test warning for use of auto in C++98 mode with C++11
 // compatibility warnings
 void f()
 {
-  auto int x = 5; // { dg-warning "changes meaning" }
+  auto int x = 5; /* { dg-warning "changes meaning" }
+  { dg-begin-multiline-output "" }
+   auto int x = 5;
+   ^~~~
+   
+  { dg-end-multiline-output "" } */
 }
===

[PATCH] C++ parser: two fix-it hints for broken member declarations

[Volker Reichelt]

Hi,

the following patch adds fix-it hints to the C++ parser for a stray
comma and a missing semicolon at the end of a member declaration.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-04-29  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_member_declaration): Add fix-it hints for
stray comma and missing semicolon at end of member declaration.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c 2017-04-28
+++ gcc/cp/parser.c 2017-04-28
@@ -23461,8 +23461,10 @@
  if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
{
  cp_token *token = cp_lexer_previous_token (parser->lexer);
- error_at (token->location,
-   "stray %<,%> at end of member declaration");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_remove ();
+ error_at_rich_loc (, "stray %<,%> at end of "
+"member declaration");
}
}
  /* If the next token isn't a `;', then we have a parse error.  */
@@ -23473,8 +23475,10 @@
 actual semicolon is missing.  Find the previous token
 and use that for our error position.  */
  cp_token *token = cp_lexer_previous_token (parser->lexer);
- error_at (token->location,
-   "expected %<;%> at end of member declaration");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_insert_after (";");
+ error_at_rich_loc (, "expected %<;%> at end of "
+"member declaration");
 
  /* Assume that the user meant to provide a semicolon.  If
 we were to cp_parser_skip_to_end_of_statement, we might
===

2017-04-29  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/diagnostic/member-decl-1.C: New test.

Index: gcc/testsuite/g++.dg/diagnostic/member-decl-1.C
===
--- gcc/testsuite/g++.dg/diagnostic/member-decl-1.C 2017-04-29
+++ gcc/testsuite/g++.dg/diagnostic/member-decl-1.C 2017-04-29
@@ -0,0 +1,18 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+struct A
+{
+  int i,;  /* { dg-error "stray .,. at end of member declaration" }
+  { dg-begin-multiline-output "" }
+   int i,;
+^
+-
+  { dg-end-multiline-output "" } */
+
+  int j  /* { dg-error "expected .;. at end of member declaration" }
+  { dg-begin-multiline-output "" }
+   int j
+   ^
+;
+  { dg-end-multiline-output "" } */
+};
===

Re: [PATCH] Improved diagnostics for casts and enums

[Volker Reichelt]

On 27 Apr, Martin Sebor wrote:
> On 04/27/2017 01:29 AM, Volker Reichelt wrote:
>> Hi,
>>
>> the following two patches aim at improving GCC's diagnostics to help
>> the user to get rid of old-style casts. While old-style pointer casts
>> are really bad and need to be weeded out quickly, old-style casts between
>> arithmetic types are IMHO much more tolerable. The patches allow to
>> easily distinguish between those situations.
> 
> FWIW, it can be most helpful to include this sort of detail (and
> similar) in diagnostics.  In the case of the C-style cast, besides
> mentioning the type of the result, it might be even more helpful
> to mention the type of the operand because unlike that of the
> result, its type is not apparent from the cast itself.

In this particular case the operand is evaluated after the warning
message. So I couldn't include it in the message without fiddling
around with the logic. I was also afraid that the warning message would
become too long.

>> The first patch for cp_parser_cast_expression in parser.c just adds
>> the target type of the cast to the diagnostic (like in
>> maybe_warn_about_useless_cast in typeck.c).
>>
>> The second patch for type_to_string in error.c tackles the problem
>> that the name of a type doesn't tell you if you have a class or just
>> a simple enum. Similar to adding "{aka 'int'}" to types that
>> are essentially integers, this patch adds "{enum}" to all
>> enumeration types (and adjusts two testcases accordingly).
> 
> In the C front end %qT prints 'enum E' for an argument of
> an enumerated type.  Is there some significance to having
> the C++ front end print 'E { enum }' or can C++ be made
> consistent?
>
> Martin

Ah, good point! I was so focused on the '{aka ...}' thing that I didn't
see the obvious: I should have used %q#T instead of just %qT to get
the enum prefix with the type. Unfortunately, I already committed the
patch after Nathan's OK before seeing your message.

I'll prepare another patch this weekend to revert my error.c changes and
use %q#T for the warnings about old-style and useless casts mentioned
above.

Another thought just crossed my mind: Other users might find an
enum/class prefix helpful for different warnings. Right now they
would have to modify the compile to use %q#T instead of %qT.
Would it make sense to add a compiler option that sets the verbose
flag for cp_printer unconditionally? Or am I just unaware of existing
functionality?

Regards,
Volker

[PATCH] Improved diagnostics for casts and enums

[Volker Reichelt]

Hi,

the following two patches aim at improving GCC's diagnostics to help
the user to get rid of old-style casts. While old-style pointer casts
are really bad and need to be weeded out quickly, old-style casts between
arithmetic types are IMHO much more tolerable. The patches allow to
easily distinguish between those situations.

The first patch for cp_parser_cast_expression in parser.c just adds
the target type of the cast to the diagnostic (like in
maybe_warn_about_useless_cast in typeck.c).

The second patch for type_to_string in error.c tackles the problem
that the name of a type doesn't tell you if you have a class or just
a simple enum. Similar to adding "{aka 'int'}" to types that
are essentially integers, this patch adds "{enum}" to all
enumeration types (and adjusts two testcases accordingly).

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk (as two patches or as one)?

Regards,
Volker


2017-04-27  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_cast_expression): Add target type of cast to
diagnostic.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247283)
+++ gcc/cp/parser.c (working copy)
@@ -8763,7 +8763,8 @@
  && !in_system_header_at (input_location)
  && !VOID_TYPE_P (type)
  && current_lang_name != lang_name_c)
-   warning (OPT_Wold_style_cast, "use of old-style cast");
+   warning (OPT_Wold_style_cast,
+"use of old-style cast to %qT", type);
 
  /* Only type conversions to integral or enumeration types
 can be used in constant-expressions.  */
=======

2017-04-27  Volker Reichelt  <v.reich...@netcologne.de>

* error.c (type_to_string): Add '{enum}' suffix to enumeration types.

Index: gcc/cp/error.c
===
--- gcc/cp/error.c  (revision 247283)
+++ gcc/cp/error.c  (working copy)
@@ -3134,6 +3134,10 @@
   if (len == aka_len && memcmp (p, p+aka_start, len) == 0)
p[len] = '\0';
 }
+
+  if (typ && TYPE_P (typ) && TREE_CODE (typ) == ENUMERAL_TYPE)
+pp_string (cxx_pp, M_(" {enum}"));
+
   return pp_ggc_formatted_text (cxx_pp);
 }
 
===

2017-04-27  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/cpp1z/direct-enum-init1.C: Adjust for more verbose enum
diagnostics.
* g++.dg/warn/pr12242.C: Likewise.

Index: gcc/testsuite/g++.dg/cpp1z/direct-enum-init1.C
===
--- gcc/testsuite/g++.dg/cpp1z/direct-enum-init1.C  (revision 247283)
+++ gcc/testsuite/g++.dg/cpp1z/direct-enum-init1.C  (working copy)
@@ -17,67 +17,67 @@
 void
 foo ()
 {
-  A a1 { 5 };  // { dg-error "invalid conversion from 'int' to 'A'" }
-  B b1 { 7 };  // { dg-error "invalid conversion from 'int' to 'B'" "" 
{ target c++14_down } }
+  A a1 { 5 };  // { dg-error "invalid conversion from 'int' to 'A 
{enum}'" }
+  B b1 { 7 };  // { dg-error "invalid conversion from 'int' to 'B 
{enum}'" "" { target c++14_down } }
   C c1 { s };
-  D d1 { D(t) };   // { dg-error "invalid cast from type 'T' to type 'D'" }
-  D d2 { t };  // { dg-error "cannot convert 'T' to 'D' in 
initialization" "" { target c++14_down } }
+  D d1 { D(t) };   // { dg-error "invalid cast from type 'T' to type 'D 
{enum}'" }
+  D d2 { t };  // { dg-error "cannot convert 'T' to 'D {enum}' in 
initialization" "" { target c++14_down } }
// { dg-error "invalid cast from type 'T' to type 'D'" 
"" { target c++1z } .-1 }
-  D d3 { 9 };  // { dg-error "cannot convert 'int' to 'D' in 
initialization" "" { target c++14_down } }
-  D d4 { l };  // { dg-error "cannot convert 'long int' to 'D' in 
initialization" "" { target c++14_down } }
+  D d3 { 9 };  // { dg-error "cannot convert 'int' to 'D {enum}' in 
initialization" "" { target c++14_down } }
+  D d4 { l };  // { dg-error "cannot convert 'long int' to 'D {enum}' 
in initialization" "" { target c++14_down } }
   D d5 { D(l) };
-  D d6 { G };  // { dg-error "cannot convert 'A' to 'D' in 
initialization" "" { target c++14_down } }
-  E e1 { 5 };  // { dg-error "cannot convert 'int' to 'E' in 
initialization" "" { target c++14_down } }
-  E e2 { -1 }; // { d

[PATCH, C++] Fix-it info for typo in nested-name-specifier

[Volker Reichelt]

Hi,

the following patch adds fix-it information for a diagnostic in the C++
parser: Use a scope operator instead of a single colon in a
nested-name-specifier.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-04-26  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_nested_name_specifier_opt): Add fix-it
information to diagnostic of invalid colon in nested-name-specifier.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247254)
+++ gcc/cp/parser.c (working copy)
@@ -5931,8 +5931,11 @@
  && parser->colon_corrects_to_scope_p
  && cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_NAME)
{
- error_at (token->location,
-   "found %<:%> in nested-name-specifier, expected 
%<::%>");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_replace ("::");
+ error_at_rich_loc (,
+"found %<:%> in nested-name-specifier, "
+"expected %<::%>");
  token->type = CPP_SCOPE;
}
 
===

2017-04-26  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/diagnostic/nested-name-1.C: New test.

Index: gcc/testsuite/g++.dg/diagnostic/nested-name-1.C
===
--- gcc/testsuite/g++.dg/diagnostic/nested-name-1.C 2017-04-26
+++ gcc/testsuite/g++.dg/diagnostic/nested-name-1.C 2017-04-26
@@ -0,0 +1,13 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+namespace N
+{
+  struct A {};
+}
+
+N:A a;  /* { dg-error "nested-name-specifier" }
+  { dg-begin-multiline-output "" }
+ N:A a;
+  ^
+  ::
+  { dg-end-multiline-output "" } */
===

[PATCH, C++] Fix-it info for invalid class/struct after enum

[Volker Reichelt]

Hi,

the following patch adds fix-it information for a pedwarn in the C++
parser about the invalid use of class/struct after enum.
I factored out the call to cp_lexer_peek_token, because it was called
already 3 times (twice from within cp_lexer_next_token_is_keyword)
and I didn't want to add a 4th call to get the token's location.
I also fixed the broken indentation of the pedwarn.

Thanks to David for suggesting to use gcc_rich_location::add_range
to highlight multiple tokens.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-04-25  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_elaborated_type_specifier): Add fix-it to
diagnostic of invalid class/struct keyword after enum.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247110)
+++ gcc/cp/parser.c (working copy)
@@ -17270,12 +17270,16 @@
   tag_type = enum_type;
   /* Issue a warning if the `struct' or `class' key (for C++0x scoped
 enums) is used here.  */
-  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_CLASS)
- || cp_lexer_next_token_is_keyword (parser->lexer, RID_STRUCT))
+  cp_token *token = cp_lexer_peek_token (parser->lexer);
+  if (cp_parser_is_keyword (token, RID_CLASS)
+ || cp_parser_is_keyword (token, RID_STRUCT))
{
-   pedwarn (input_location, 0, "elaborated-type-specifier "
- "for a scoped enum must not use the %qD keyword",
- cp_lexer_peek_token (parser->lexer)->u.value);
+ gcc_rich_location richloc (token->location);
+ richloc.add_range (input_location, false);
+ richloc.add_fixit_remove ();
+ pedwarn_at_rich_loc (, 0, "elaborated-type-specifier for "
+  "a scoped enum must not use the %qD keyword",
+  token->u.value);
  /* Consume the `struct' or `class' and parse it anyway.  */
  cp_lexer_consume_token (parser->lexer);
}
=======

2017-04-25  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/cpp0x/enum34.C: New test.

Index: gcc/testsuite/g++.dg/cpp0x/enum34.C
===
--- gcc/testsuite/g++.dg/cpp0x/enum34.C 2017-04-25
+++ gcc/testsuite/g++.dg/cpp0x/enum34.C 2017-04-25
@@ -0,0 +1,11 @@
+// { dg-options "-fdiagnostics-show-caret" }
+// { dg-do compile { target c++11 } }
+
+enum class E;
+
+enum class E e;  /* { dg-warning "scoped enum must not use" }
+  { dg-begin-multiline-output "" }
+ enum class E e;
+  ^
+  -
+  { dg-end-multiline-output "" } */
===

[PATCH] Simplify quoting in diagnostics of C++ frontend

[Volker Reichelt]

Hi,

the following patch simplifies quoting in diagnostics by using
%qD instead of %<%D%> etc.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

Btw, line 14563 in pt.c
  error ("enumerator value %E is outside the range of underlying "
contains an unquoted %E. Shouldn't that be replaced with %qE?

OK for trunk?

Regards,
Volker



2017-04-23  Volker Reichelt  <v.reich...@netcologne.de>

* decl.c (grokdeclarator): Use %qT instead of %<%T%> in diagnostics.
(start_enum): Likewise.
(build_enumerator): Likewise.
* parser.c (cp_parser_mem_initializer_list): Use %qD instead of
%<%D%> in diagnostics.
(cp_parser_elaborated_type_specifier): Likewise.
* pt.c (make_pack_expansion): Use %qT and %qE instead of
%<%T%> and %<%E%> in diagnostics.
(tsubst_pack_expansion): Likewise.

Index: gcc/cp/decl.c
===
--- gcc/cp/decl.c   (revision 247067)
+++ gcc/cp/decl.c   (working copy)
@@ -11433,9 +11433,9 @@
{
  error (funcdef_flag || initialized
 ? G_("cannot define member function %<%T::%s%> "
- "within %<%T%>")
+ "within %qT")
 : G_("cannot declare member function %<%T::%s%> "
- "within %<%T%>"),
+ "within %qT"),
 ctype, name, current_class_type);
  return error_mark_node;
}
@@ -14150,7 +14150,7 @@
   else if (dependent_type_p (underlying_type))
ENUM_UNDERLYING_TYPE (enumtype) = underlying_type;
   else
-error ("underlying type %<%T%> of %<%T%> must be an integral type", 
+error ("underlying type %qT of %qT must be an integral type", 
underlying_type, enumtype);
 }
 
@@ -14561,7 +14561,7 @@
 {
  if (!int_fits_type_p (value, ENUM_UNDERLYING_TYPE (enumtype)))
error ("enumerator value %E is outside the range of underlying "
-  "type %<%T%>", value, ENUM_UNDERLYING_TYPE (enumtype));
+  "type %qT", value, ENUM_UNDERLYING_TYPE (enumtype));
 
   /* Convert the value to the appropriate type.  */
   value = fold_convert (ENUM_UNDERLYING_TYPE (enumtype), value);
Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 247067)
+++ gcc/cp/parser.c (working copy)
@@ -14073,7 +14073,7 @@
   && !TYPE_P (TREE_PURPOSE (mem_initializer)))
 {
   error_at (token->location,
-   "cannot expand initializer for member %<%D%>",
+   "cannot expand initializer for member %qD",
TREE_PURPOSE (mem_initializer));
   mem_initializer = error_mark_node;
 }
@@ -17263,7 +17263,7 @@
  || cp_lexer_next_token_is_keyword (parser->lexer, RID_STRUCT))
{
pedwarn (input_location, 0, "elaborated-type-specifier "
- "for a scoped enum must not use the %<%D%> keyword",
+ "for a scoped enum must not use the %qD keyword",
  cp_lexer_peek_token (parser->lexer)->u.value);
  /* Consume the `struct' or `class' and parse it anyway.  */
  cp_lexer_consume_token (parser->lexer);
Index: gcc/cp/pt.c
===
--- gcc/cp/pt.c (revision 247067)
+++ gcc/cp/pt.c (working copy)
@@ -3701,7 +3701,7 @@
 
   if (parameter_packs == NULL_TREE)
 {
-  error ("base initializer expansion %<%T%> contains no parameter 
packs", arg);
+  error ("base initializer expansion %qT contains no parameter packs", 
arg);
   delete ppd.visited;
   return error_mark_node;
 }
@@ -3765,9 +3765,9 @@
   if (parameter_packs == NULL_TREE)
 {
   if (TYPE_P (arg))
-error ("expansion pattern %<%T%> contains no argument packs", arg);
+error ("expansion pattern %qT contains no argument packs", arg);
   else
-error ("expansion pattern %<%E%> contains no argument packs", arg);
+error ("expansion pattern %qE contains no argument packs", arg);
   return error_mark_node;
 }
   PACK_EXPANSION_PARAMETER_PACKS (result) = parameter_packs;
@@ -11409,12 +11409,10 @@
  if (!(complain & tf_error))
/* Fail quietly.  */;
   else if (TREE_CODE (t) == TYPE_PACK_EXPANSION)
-error ("mismatched argum

[C++ PATCH] Fix-it info for duplicate tokens

[Volker Reichelt]

Hi,

the following patch adds fix-it info to error messages in 4 places
where the C++ parser complains about duplicate tokens.
The fix-it infos suggest to remove the duplicates.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk?

Regards,
Volker


2017-04-21  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_cv_qualifier_seq_opt): Add fix-it info to
error message.
(cp_parser_virt_specifier_seq_opt): Likewise.
(set_and_check_decl_spec_loc): Likewise twice.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 246880)
+++ gcc/cp/parser.c (working copy)
@@ -20258,7 +20258,9 @@
 
   if (cv_quals & cv_qualifier)
{
- error_at (token->location, "duplicate cv-qualifier");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_remove ();
+ error_at_rich_loc (, "duplicate cv-qualifier");
  cp_lexer_purge_token (parser->lexer);
}
   else
@@ -20405,7 +20407,9 @@
 
   if (virt_specifiers & virt_specifier)
{
- error_at (token->location, "duplicate virt-specifier");
+ gcc_rich_location richloc (token->location);
+ richloc.add_fixit_remove ();
+ error_at_rich_loc (, "duplicate virt-specifier");
  cp_lexer_purge_token (parser->lexer);
}
   else
@@ -27665,7 +27669,11 @@
error_at (location,
  "both %<__thread%> and %<thread_local%> specified");
  else
-   error_at (location, "duplicate %qD", token->u.value);
+   {
+ gcc_rich_location richloc (location);
+ richloc.add_fixit_remove ();
+ error_at_rich_loc (, "duplicate %qD", token->u.value);
+   }
}
   else
{
@@ -27686,8 +27694,9 @@
 "constexpr",
"__complex"
  };
- error_at (location,
-   "duplicate %qs", decl_spec_names[ds]);
+ gcc_rich_location richloc (location);
+     richloc.add_fixit_remove ();
+ error_at_rich_loc (, "duplicate %qs", decl_spec_names[ds]);
}
 }
 }

2017-04-21  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/diagnostic/duplicate1.C: New test.
* g++.dg/cpp0x/duplicate1.C: New test.

Index: gcc/testsuite/g++.dg/diagnostic/duplicate1.C
===
--- gcc/testsuite/g++.dg/diagnostic/duplicate1.C2017-04-21
+++ gcc/testsuite/g++.dg/diagnostic/duplicate1.C2017-04-21
@@ -0,0 +1,18 @@
+// { dg-options "-fdiagnostics-show-caret" }
+
+struct A
+{
+  void foo() const const;  /* { dg-error "duplicate cv-qualifier" }
+  { dg-begin-multiline-output "" }
+   void foo() const const;
+^
+-
+  { dg-end-multiline-output "" } */
+};
+
+volatile volatile int i = 0;  /* { dg-error "duplicate" }
+  { dg-begin-multiline-output "" }
+ volatile volatile int i = 0;
+  ^~~~
+  
+  { dg-end-multiline-output "" } */
Index: gcc/testsuite/g++.dg/cpp0x/duplicate1.C
===
--- gcc/testsuite/g++.dg/cpp0x/duplicate1.C 2017-04-21
+++ gcc/testsuite/g++.dg/cpp0x/duplicate1.C 2017-04-21
@@ -0,0 +1,29 @@
+// { dg-options "-fdiagnostics-show-caret" }
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  virtual void foo() const;
+};
+
+struct B final final : A  /* { dg-error "duplicate virt-specifier" }
+  { dg-begin-multiline-output "" }
+ struct B final final : A
+^
+-
+  { dg-end-multiline-output "" } */
+{
+  virtual void foo() const override final override;  /* { dg-error "duplicate 
virt-specifier" }
+  { dg-begin-multiline-output "" }
+   virtual void foo() const override final override;
+   ^~~~
+   
+  { dg-end-multiline-output "" } */
+};
+
+thread_local thread_local int i = 0;  /* { dg-error "duplicate" }
+  { dg-begin-multiline-output "" }
+ thread_local thread_local int i = 0;
+  ^~~~
+  
+  { dg-end-multiline-output "" } */
===

Re: [C++ PATCH] New warning for extra semicolons after in-class function definitions

[Volker Reichelt]

On  7 Apr, David Malcolm wrote:
> On Fri, 2017-04-07 at 21:55 +0200, Volker Reichelt wrote:
>> Hi,
>> 
>> with the following patch I suggest to add a diagnostic for extra
>> semicolons after in-class member function definitions:
>> 
>>   struct A
>>   {
>> A() {};
>> void foo() {};
>> friend void bar() {};
>>   };
>> 
>> Although they are allowed in the C++ standard, people (including me)
>> often like to get rid of them for stylistic/consistency reasons.
>> In fact clang has a warning -Wextra-semi for this.
>> 
>> Also in GCC (almost exactly 10 years ago) there was a patch
>> https://gcc.gnu.org/ml/gcc-cvs/2007-03/msg00841.html
>> to issue a pedwarn (which had to be reverted as GCC would reject
>> valid
>> code because of the pedwarn).
>> 
>> Instead of using pewarn the patch below adds a new warning (named
>> like
>> clang's) to warn about these redundant semicolons.
>> Btw, clang's warning message "extra ';' after member function
>> definition"
>> is slightly incorrect because it is also emitted for friend functions
>> which are not member-functions. That's why I suggest a different
>> wording:
>> 
>>   Wextra-semi.C:3:9: warning: extra ';' after in-class function
>> definition [-Wextra-semi]
>>  A() {};
>>^
>>   Wextra-semi.C:4:16: warning: extra ';' after in-class function
>> definition [-Wextra-semi]
>>  void foo() {};
>>   ^
>>   Wextra-semi.C:5:23: warning: extra ';' after in-class function
>> definition [-Wextra-semi]
>>  friend void bar() {};
>>  ^
>> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> 
>> OK for stage 1, once GCC 7 has branched?
>> Regards,
>> Volker
>> 
>> 
>> 2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>> * c.opt (Wextra-semi): New C++ warning flag.
>> 
>> Index: gcc/c-family/c.opt
>> ===
>> --- gcc/c-family/c.opt  (revision 246752)
>> +++ gcc/c-family/c.opt  (working copy)
>> @@ -504,6 +504,10 @@
>>  C ObjC C++ ObjC++ Warning
>>  ; in common.opt
>>  
>> +Wextra-semi
>> +C++ Var(warn_extra_semi) Warning
>> +Warn about semicolon after in-class function definition.
>> +
>>  Wfloat-conversion
>>  C ObjC C++ ObjC++ Var(warn_float_conversion) Warning LangEnabledBy(C
>> ObjC C++ ObjC++,Wconversion)
>>  Warn for implicit type conversions that cause loss of floating point
>> precision.
>> 
>> 2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>> * parser.c (cp_parser_member_declaration): Add warning for
>> extra semicolon after in-class function definition.
>> 
>> Index: gcc/cp/parser.c
>> ===
>> --- gcc/cp/parser.c (revision 246752)
>> +++ gcc/cp/parser.c (working copy)
>> @@ -23386,7 +23386,11 @@
>>   token = cp_lexer_peek_token (parser->lexer);
>>   /* If the next token is a semicolon, consume it. 
>> */
>>   if (token->type == CPP_SEMICOLON)
>> -   cp_lexer_consume_token (parser->lexer);
>> +   {
>> + cp_lexer_consume_token (parser->lexer);
>> + warning (OPT_Wextra_semi, "extra %<;%> "
>> +  "after in-class function definition");
>> +   }
> 
> Thanks for posting this.
> 
> I'm not a C++ maintainer, but I like the idea (though the patch is
> missing at least a doc/invoke.texi change).

You're right. I had the nagging feeling that I forgot something.
I added a respective section to doc/invoke.texi after -Wempty-body
and -Wenum-compare.


> A small improvement to this would be to emit a deletion fix-it hint
> about the redundant token (so that IDEs have a change of fixing it
> easily).
> 
> This could be done something like this:
> 
> location_t semicolon_loc
>= cp_lexer_consume_token (parser->lexer)->location;
> gcc_rich_location richloc (semicolon_loc);
> richloc.add_fixit_remove ();
> warning_at_richloc (, OPT_Wextra_semi,
> "extra %<;%> after in-class function
> definition");
> 

That's a nice suggestion. I modified the patch accordingly.


>>   goto out;
>> }
>>   

[C++ PATCH] New warning for extra semicolons after in-class function definitions

[Volker Reichelt]

Hi,

with the following patch I suggest to add a diagnostic for extra
semicolons after in-class member function definitions:

  struct A
  {
A() {};
void foo() {};
friend void bar() {};
  };

Although they are allowed in the C++ standard, people (including me)
often like to get rid of them for stylistic/consistency reasons.
In fact clang has a warning -Wextra-semi for this.

Also in GCC (almost exactly 10 years ago) there was a patch
https://gcc.gnu.org/ml/gcc-cvs/2007-03/msg00841.html
to issue a pedwarn (which had to be reverted as GCC would reject valid
code because of the pedwarn).

Instead of using pewarn the patch below adds a new warning (named like
clang's) to warn about these redundant semicolons.
Btw, clang's warning message "extra ';' after member function definition"
is slightly incorrect because it is also emitted for friend functions
which are not member-functions. That's why I suggest a different wording:

  Wextra-semi.C:3:9: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 A() {};
   ^
  Wextra-semi.C:4:16: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 void foo() {};
  ^
  Wextra-semi.C:5:23: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 friend void bar() {};
 ^

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for stage 1, once GCC 7 has branched?

Regards,
Volker


2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Wextra-semi): New C++ warning flag.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 246752)
+++ gcc/c-family/c.opt  (working copy)
@@ -504,6 +504,10 @@
 C ObjC C++ ObjC++ Warning
 ; in common.opt
 
+Wextra-semi
+C++ Var(warn_extra_semi) Warning
+Warn about semicolon after in-class function definition.
+
 Wfloat-conversion
 C ObjC C++ ObjC++ Var(warn_float_conversion) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wconversion)
 Warn for implicit type conversions that cause loss of floating point precision.

2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_member_declaration): Add warning for
extra semicolon after in-class function definition.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 246752)
+++ gcc/cp/parser.c (working copy)
@@ -23386,7 +23386,11 @@
  token = cp_lexer_peek_token (parser->lexer);
  /* If the next token is a semicolon, consume it.  */
  if (token->type == CPP_SEMICOLON)
-   cp_lexer_consume_token (parser->lexer);
+   {
+ cp_lexer_consume_token (parser->lexer);
+ warning (OPT_Wextra_semi, "extra %<;%> "
+  "after in-class function definition");
+   }
  goto out;
    }
  else

2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/warn/Wextra-semi.C: New test.

Index: gcc/testsuite/g++.dg/warn/Wextra-semi.C
===
--- gcc/testsuite/g++.dg/warn/Wextra-semi.C 2017-04-07
+++ gcc/testsuite/g++.dg/warn/Wextra-semi.C 2017-04-07
@@ -0,0 +1,8 @@
+// { dg-options "-Wextra-semi" }
+
+struct A
+{
+  A() {};  // { dg-warning "after in-class function definition" }
+  void foo() {};   // { dg-warning "after in-class function definition" }
+  friend void bar() {};// { dg-warning "after in-class function 
definition" }
+};
===

[C++ PATCH] New warning for extra semicolons after in-class function definitions

[Volker Reichelt]

Hi,

with the following patch I suggest to add a diagnostic for extra
semicolons after in-class member function definitions:

  struct A
  {
A() {};
void foo() {};
friend void bar() {};
  };

Although they are allowed in the C++ standard, people (including me)
often like to get rid of them for stylistic/consistency reasons.
In fact clang has a warning -Wextra-semi for this.

Also in GCC (almost exactly 10 years ago) there was a patch
https://gcc.gnu.org/ml/gcc-cvs/2007-03/msg00841.html
to issue a pedwarn (which had to be reverted as GCC would reject valid
code because of the pedwarn).

Instead of using pewarn the patch below adds a new warning (named like
clang's) to warn about these redundant semicolons.
Btw, clang's warning message "extra ';' after member function definition"
is slightly incorrect because it is also emitted for friend functions
which are not member-functions. That's why I suggest a different wording:

  Wextra-semi.C:3:9: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 A() {};
   ^
  Wextra-semi.C:4:16: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 void foo() {};
  ^
  Wextra-semi.C:5:23: warning: extra ';' after in-class function definition 
[-Wextra-semi]
 friend void bar() {};
 ^

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for stage 1, once GCC 7 has branched?

Regards,
Volker


2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* c.opt (Wextra-semi): New C++ warning flag.

Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt  (revision 246752)
+++ gcc/c-family/c.opt  (working copy)
@@ -504,6 +504,10 @@
 C ObjC C++ ObjC++ Warning
 ; in common.opt
 
+Wextra-semi
+C++ Var(warn_extra_semi) Warning
+Warn about semicolon after in-class function definition.
+
 Wfloat-conversion
 C ObjC C++ ObjC++ Var(warn_float_conversion) Warning LangEnabledBy(C ObjC C++ 
ObjC++,Wconversion)
 Warn for implicit type conversions that cause loss of floating point precision.

2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* parser.c (cp_parser_member_declaration): Add warning for
extra semicolon after in-class function definition.

Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 246752)
+++ gcc/cp/parser.c (working copy)
@@ -23386,7 +23386,11 @@
  token = cp_lexer_peek_token (parser->lexer);
  /* If the next token is a semicolon, consume it.  */
  if (token->type == CPP_SEMICOLON)
-   cp_lexer_consume_token (parser->lexer);
+   {
+ cp_lexer_consume_token (parser->lexer);
+ warning (OPT_Wextra_semi, "extra %<;%> "
+  "after in-class function definition");
+   }
  goto out;
    }
  else

2017-04-07  Volker Reichelt  <v.reich...@netcologne.de>

* g++.dg/warn/Wextra-semi.C: New test.

Index: gcc/testsuite/g++.dg/warn/Wextra-semi.C
===
--- gcc/testsuite/g++.dg/warn/Wextra-semi.C 2017-04-07
+++ gcc/testsuite/g++.dg/warn/Wextra-semi.C 2017-04-07
@@ -0,0 +1,8 @@
+// { dg-options "-Wextra-semi" }
+
+struct A
+{
+  A() {};  // { dg-warning "after in-class function definition" }
+  void foo() {};   // { dg-warning "after in-class function definition" }
+  friend void bar() {};// { dg-warning "after in-class function 
definition" }
+};
===

[Patch C++/80296] Fix broken diagnostic: 'unary_plus_expr' not supported by expression

[Volker Reichelt]

The following patch fixes a broken diagnostic:
#'unary_plus_expr' not supported by expression#

The code to handle UNARY_PLUS_EXPR is already in place in
cxx_pretty_printer::unary_expression. However, UNARY_PLUS_EXPR
is not checked in cxx_pretty_printer::expression, so that we
don't call cxx_pretty_printer::unary_expression.
Fixed with the patch below.

Bootstrapped and regtested on x86_64-pc-linux-gnu.

OK for trunk? Or should this wait for stage 1?

Regards,
Volker


2017-04-03  Volker Reichelt  <v.reich...@netcologne.de>

PR c++/80296
* cxx-pretty-print.c (cxx_pretty_printer::expression): Add
UNARY_PLUS_EXPR case.

Index: gcc/cp/cxx-pretty-print.c
===
--- gcc/cp/cxx-pretty-print.c   (revision 246620)
+++ gcc/cp/cxx-pretty-print.c   (working copy)
@@ -1112,6 +1112,7 @@
 case SIZEOF_EXPR:
 case ALIGNOF_EXPR:
 case NOEXCEPT_EXPR:
+case UNARY_PLUS_EXPR:
   unary_expression (t);
   break;
 
2017-04-03  Volker Reichelt  <v.reich...@netcologne.de>

PR c++/80296
* g++.dg/cpp0x/alias-decl-80296.C: New test.

Index: gcc/testsuite/g++.dg/cpp0x/alias-decl-80296.C
===
--- gcc/testsuite/g++.dg/cpp0x/alias-decl-80296.C   2017-04-03
+++ gcc/testsuite/g++.dg/cpp0x/alias-decl-80296.C   2017-04-03
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++11 } }
+// { dg-bogus "not supported by" "" { target *-*-* } 0 }
+
+template  struct A {};
+
+template  using B = A<+N...>;
+
+B b; // { dg-error "type/value mismatch" }
+  // { dg-message "expected a constant" "expected" { target *-*-* } 8 }
===

[PATCH] Pretty-printing of some unsupported expressions (PR c/35441)

[Volker Reichelt]

The patch below addresses some pretty-printer issues. More precisely,
it handles 6 types of expressions that weren't handled before:

  a) EXACT_DIV_EXPR, RDIV_EXPR,
  b) LROTATE_EXPR, RROTATE_EXPR,
  c) MAX_EXPR, MIN_EXPR.

Right now we print for the testcase below:

  #'exact_div_expr' not supported by expression#'pr35441.c: In function 'foo1':
  pr35441.c:8:6: error: called object  is not a function or function pointer
  #'rdiv_expr' not supported by expression#'pr35441.c: In function 'foo2':
  pr35441.c:13:5: error: called object  is not a function or function pointer
  #'lrotate_expr' not supported by expression#'pr35441.c: In function 'foo3':
  pr35441.c:18:11: error: called object  is not a function or function pointer
  #'rrotate_expr' not supported by expression#'pr35441.c:19:11: error: called 
object  is not a function or function pointer
  #'min_expr' not supported by expression#'pr35441.c: In function 'foo4':
  pr35441.c:24:14: error: called object  is not a function or function pointer
  #'max_expr' not supported by expression#'pr35441.c:25:14: error: called 
object  is not a function or function pointer

With the patch we would print:

  pr35441.c: In function 'foo1':
  pr35441.c:8:6: error: called object '((long int)p - (long int)q) / 8' is not 
a function or function pointer
  pr35441.c: In function 'foo2':
  pr35441.c:13:5: error: called object 'x / y' is not a function or function 
pointer
  pr35441.c: In function 'foo3':
  pr35441.c:18:11: error: called object 'i << j' is not a function or function 
pointer
  pr35441.c:19:11: error: called object 'i >> j' is not a function or function 
pointer
  pr35441.c: In function 'foo4':
  pr35441.c:24:14: error: called object 'min(q,  p)' is not a function or 
function pointer
  pr35441.c:25:14: error: called object 'max(q,  p)' is not a function or 
function pointer


a) This part (with foo1 and foo2 from the testcase) is straightforward.

However, I'm not sure how to print the actual operators for the other
2 groups.

b) I chose the shift operators 'a << b' and 'a >> b' for the rotate
   expressions, which is not 100% correct. Would it be better to use
   something like 'lrotate(a, b)', '__lrotate__(a, b)' or 'a lrotate b'
   instead? Or is there something like an '__builtin_lrotate' that I misseed?

c) I chose 'max(q, b)' and 'min(q, b)'.
   Would something like '__max__(q, b)' be better?
   Trying to print the expression 'p < q ? p : q' from the testcase
   might be possible. But there are several different expressions like
   'p < q ? p : q', 'p > q ? q : p', 'p >= q ? q : p' and '!(p >= q) ? p : q'
   that are all converted into MIN_EXPR, so I would rather stay with
   and explicit 'min'/'max' instead.

In addition I found some more division operators in gcc/tree.def that
aren't handled by the pretty-printer:

  CEIL_DIV_EXPR
  FLOOR_DIV_EXPR
  ROUND_DIV_EXPR
  CEIL_MOD_EXPR
  FLOOR_MOD_EXPR
  ROUND_MOD_EXPR

Alas I don't have testcases for them. Nevertheless, I could handle them
like the other MOD and DIV operators just to be safe.

Any comments, or is the patch OK as is for trunk?
Bootstrapped and regtested on x86_64-linux.

Regards,
Volker


2017-03-09  Volker Reichelt  <v.reich...@netcologne.de>

PR c/35441
* c-pretty-print.c (c_pretty_printer::expression): Handle MAX_EXPR,
MIN_EXPR, EXACT_DIV_EXPR, RDIV_EXPR, LROTATE_EXPR, RROTATE_EXPR.
(c_pretty_printer::postfix_expression): Handle MAX_EXPR, MIN_EXPR.
(c_pretty_printer::multiplicative_expression): Handle EXACT_DIV_EXPR,
RDIV_EXPR.
(pp_c_shift_expression): Handle LROTATE_EXPR, RROTATE_EXPR.

Index: gcc/c-family/c-pretty-print.c
===
--- gcc/c-family/c-pretty-print.c   (revision 245962)
+++ gcc/c-family/c-pretty-print.c   (working copy)
@@ -1551,6 +1551,14 @@
   : "__builtin_islessgreater");
   goto two_args_fun;
 
+case MAX_EXPR:
+  pp_c_ws_string (this, "max");
+  goto two_args_fun;
+
+case MIN_EXPR:
+  pp_c_ws_string (this, "min");
+  goto two_args_fun;
+
 two_args_fun:
   pp_c_left_paren (this);
   expression (TREE_OPERAND (e, 0));
@@ -1829,6 +1837,8 @@
 case MULT_EXPR:
 case TRUNC_DIV_EXPR:
 case TRUNC_MOD_EXPR:
+case EXACT_DIV_EXPR:
+case RDIV_EXPR:
   multiplicative_expression (TREE_OPERAND (e, 0));
   pp_c_whitespace (this);
   if (code == MULT_EXPR)
@@ -1890,9 +1900,12 @@
 {
 case LSHIFT_EXPR:
 case RSHIFT_EXPR:
+case LROTATE_EXPR:
+case RROTATE_EXPR:
   pp_c_shift_expression (pp, TREE_OPERAND (e, 0));
   pp_c_whitespace (pp);
-  pp_string (pp, code == LSHIFT_EXPR ? "<<" : ">>");
+  pp_string (pp,
+code == LSHIFT_EXPR || code == LROTATE_EXPR ? "<<" : ">>");
   pp_c_whi

Re: [PATCH] Some more translation related tweaks

[Volker Reichelt]

On 27 Feb, Jakub Jelinek wrote:
> On Mon, Feb 27, 2017 at 11:04:36AM +0100, Volker Reichelt wrote:
>> So here's the second attempt:
>> 
>> 2017-02-27  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>>  * init.c: Include intl.h.
>>  (build_new_1): Move message strings into pedwarn to make them
>>  -Wformat-security friendly. Mark string for translation.
>>  * pt.c (tsubst_copy_and_build): Mark string for translation.
>>  Make the pointer const.
>>  * semantics.c (finish_id_expression): Mark strings for translation.
> 
> Ok for trunk then, I'll deal with the rest I've mentioned in another mail
> myself.
> 
>   Jakub

Ok, committed as revision 245757.

Volker

Re: [PATCH] Some more translation related tweaks

[Volker Reichelt]

On 26 Feb, Jakub Jelinek wrote:
> On Sun, Feb 26, 2017 at 01:18:57PM +0100, Volker Reichelt wrote:
>> 2017-02-26  Volker Reichelt  <v.reich...@netcologne.de>
>> 
>>  * init.c: Include intl.h
> 
> Missing .

Indeed, I noticed that one after I hit the send button.

>> @@ -29,6 +29,7 @@
>>  #include "varasm.h"
>>  #include "gimplify.h"
>>  #include "c-family/c-ubsan.h"
>> +#include "intl.h"
>>  
>>  static bool begin_init_stmts (tree *, tree *);
>>  static tree finish_init_stmts (bool, tree, tree);
>> @@ -2805,11 +2806,11 @@
>>  {
>>const char *msg;
>>if (typedef_variant_p (orig_type))
>> -msg = ("non-constant array new length must be specified "
>> -   "directly, not by typedef");
>> +msg = G_("non-constant array new length must be specified "
>> + "directly, not by typedef");
>>else
>> -msg = ("non-constant array new length must be specified "
>> -   "without parentheses around the type-id");
>> +msg = G_("non-constant array new length must be specified "
>> + "without parentheses around the type-id");
>>pedwarn (EXPR_LOC_OR_LOC (outer_nelts, input_location),
>> OPT_Wvla, msg);
> 
> This is not -Wformat-security friendly, perhaps better
> pedwarn (EXPR_LOC_OR_LOC (outer_nelts, input_location), OPT_Wvla,
>  typedef_variant_p (orig_type)
>  ? "non-constant array new length must be specified "
>"directly, not by typedef"
>  : "non-constant array new length must be specified "
>"without parentheses around the type-id");
> ?

Not quite. Like this the second string doesn't end up in the gcc.pot
file for translation. I had to wrap the second string in G_(...) to make
it work. (I'll have a look for other instances of this pattern and
prepare a separate patch.)

>>  }
>> Index: gcc/cp/pt.c
>> ===
>> --- gcc/cp/pt.c  (revision 245719)
>> +++ gcc/cp/pt.c  (working copy)
>> @@ -17190,10 +17190,11 @@
>> stricter.  */
>>  bool in_lambda = (current_class_type
>>&& LAMBDA_TYPE_P (current_class_type));
>> -char const *msg = "%qD was not declared in this scope, "
>> -  "and no declarations were found by "
>> -  "argument-dependent lookup at the point "
>> -  "of instantiation";
>> +char const *const msg =
> 
> = should go on the next line in this case, i.e.
> = G_("%qD was not declared in this scope, "

Indeed, thanks for noticing.

> 
>   Jakub

So here's the second attempt:

2017-02-27  Volker Reichelt  <v.reich...@netcologne.de>

* init.c: Include intl.h.
(build_new_1): Move message strings into pedwarn to make them
-Wformat-security friendly. Mark string for translation.
* pt.c (tsubst_copy_and_build): Mark string for translation.
Make the pointer const.
* semantics.c (finish_id_expression): Mark strings for translation.

Index: gcc/cp/init.c
===
--- gcc/cp/init.c   (revision 245719)
+++ gcc/cp/init.c   (working copy)
@@ -29,6 +29,7 @@
 #include "varasm.h"
 #include "gimplify.h"
 #include "c-family/c-ubsan.h"
+#include "intl.h"
 
 static bool begin_init_stmts (tree *, tree *);
 static tree finish_init_stmts (bool, tree, tree);
@@ -2803,15 +2804,12 @@
 {
   if (complain & tf_warning_or_error)
{
- const char *msg;
- if (typedef_variant_p (orig_type))
-   msg = ("non-constant array new length must be specified "
-  "directly, not by typedef");
- else
-   msg = ("non-constant array new length must be specified "
-  "without parentheses around the type-id");
- pedwarn (EXPR_LOC_OR_LOC (outer_nelts, input_location),
-  OPT_Wvla, msg);
+ pedwarn (EXPR_LOC_OR_LOC (outer_nelts, input_location), OPT_Wvla,
+  typedef_variant_p (orig_type)
+  ? "non-constant array new length must be specified "
+"directly, not by typedef&q

[PATCH] Some more translation related tweaks

[Volker Reichelt]

After Jakub's and Marek's latest translation patches, I grepped for
other unmarked messages. Here's what I found in the C++ front-end.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

Regards,
Volker


2017-02-26  Volker Reichelt  <v.reich...@netcologne.de>

* init.c: Include intl.h
(build_new_1): Mark strings for translation.
* pt.c (tsubst_copy_and_build): Mark string for translation.
Make the pointer const.
* semantics.c (finish_id_expression): Mark strings for translation.

Index: gcc/cp/init.c
===
--- gcc/cp/init.c   (revision 245719)
+++ gcc/cp/init.c   (working copy)
@@ -29,6 +29,7 @@
 #include "varasm.h"
 #include "gimplify.h"
 #include "c-family/c-ubsan.h"
+#include "intl.h"
 
 static bool begin_init_stmts (tree *, tree *);
 static tree finish_init_stmts (bool, tree, tree);
@@ -2805,11 +2806,11 @@
{
  const char *msg;
  if (typedef_variant_p (orig_type))
-   msg = ("non-constant array new length must be specified "
-  "directly, not by typedef");
+   msg = G_("non-constant array new length must be specified "
+"directly, not by typedef");
  else
-   msg = ("non-constant array new length must be specified "
-  "without parentheses around the type-id");
+   msg = G_("non-constant array new length must be specified "
+"without parentheses around the type-id");
  pedwarn (EXPR_LOC_OR_LOC (outer_nelts, input_location),
   OPT_Wvla, msg);
}
Index: gcc/cp/pt.c
===
--- gcc/cp/pt.c (revision 245719)
+++ gcc/cp/pt.c (working copy)
@@ -17190,10 +17190,11 @@
   stricter.  */
bool in_lambda = (current_class_type
  && LAMBDA_TYPE_P (current_class_type));
-   char const *msg = "%qD was not declared in this scope, "
- "and no declarations were found by "
- "argument-dependent lookup at the point "
- "of instantiation";
+   char const *const msg =
+ G_("%qD was not declared in this scope, "
+"and no declarations were found by "
+"argument-dependent lookup at the point "
+"of instantiation");
 
bool diag = true;
if (in_lambda)
Index: gcc/cp/semantics.c
===
--- gcc/cp/semantics.c  (revision 245719)
+++ gcc/cp/semantics.c  (working copy)
@@ -3510,7 +3510,7 @@
  && DECL_CONTEXT (decl) == NULL_TREE
  && !cp_unevaluated_operand)
{
- *error_msg = "use of parameter outside function body";
+ *error_msg = G_("use of parameter outside function body");
  return error_mark_node;
}
 }
@@ -3520,13 +3520,13 @@
   if (TREE_CODE (decl) == TEMPLATE_DECL
   && !DECL_FUNCTION_TEMPLATE_P (decl))
 {
-  *error_msg = "missing template arguments";
+  *error_msg = G_("missing template arguments");
   return error_mark_node;
 }
   else if (TREE_CODE (decl) == TYPE_DECL
   || TREE_CODE (decl) == NAMESPACE_DECL)
 {
-  *error_msg = "expected primary-expression";
+  *error_msg = G_("expected primary-expression");
   return error_mark_node;
 }
 
===

C++ warnings vs. errors

[Volker Reichelt]

Hi,

since Manuel's patch http://gcc.gnu.org/ml/gcc-patches/2008-02/msg00962.html
a lot of C++ code is now accepted on mainline (when compiling without
special flags like -fpermissive and -pedantic), that used to be rejected.
Instead of getting closer to the standard we get away from it, which is a
bad idea IMHO - especially since the standard should be widely adopted by
now, given that it's about 10 years old. So here's a collection of some
warnings that I'd rather see as errors:


* Scopes in for-loops:

  void foo()
  {
for (int i=0; i10; ++i) {}
i = 0;
  }

  warn.cc: In function 'void foo()':
  warn.cc:4: warning: name lookup of 'i' changed for new ISO 'for' scoping
  warn.cc:3: warning:   using obsolete binding at 'i'

  Btw, because the compiler tries to be smart to track new scoping and old
  scoping at once it rejects valid code, accepts invalid code and even
  generates wrong code in some cases (see PR10852).


* Declaration with no type:

  foo() {}

  warn.cc:1: warning: ISO C++ forbids declaration of 'foo' with no type


  Or even worse IMHO:

  struct A
  {
i;
  };

  warn.cc:3: warning: ISO C++ forbids declaration of 'i' with no type


* Invalid use of 'template':

  struct A
  {
static void foo();
  };

  templateint void bar()
  {
A::template foo();
  }

  warn.cc: In function 'void bar()':
  warn.cc:8: warning: 'A::foo()' is not a template

  Btw, I don't know why we should accept this even with -fpermissive.


* Using 'struct' for a union:

  union A {};
  struct A a;

  warn.cc:2: warning: 'struct' tag used in naming 'union A'


* Static members of local classes:

  void foo()
  {
struct A
{
  static int i;
};
  }

  warn.cc: In function 'void foo()':
  warn.cc:5: warning: local class 'struct foo()::A' shall not have static data 
member 'int foo()::A::i'


* Return without value:

  int foo()
  {
return;
  }

  warn.cc: In function 'int foo()':
  warn.cc:1: warning: return-statement with no value, in function returning 
'int'


* Definition in wrong namespace:

  struct A
  {
void foo();
  };

  namespace N
  {
void A::foo() {}
  }

  warn.cc:8: warning: definition of 'void A::foo()' is not in namespace 
enclosing 'A'


* Operator new:

  struct A
  {
void* operator new(char);
  };

  warn.cc:3: warning: 'operator new' takes type 'size_t' ('unsigned int') as 
first parameter


* Sizeof for function types:

  void foo() { sizeof(foo); }

  warn.cc: In member function 'void A::foo()':
  warn.cc:1: warning: ISO C++ forbids applying 'sizeof' to an expression of 
function type


What do you think?

Regards,
Volker

Priorites for PR 35435 and PR 35441

[Volker Reichelt]

Hi Joseph,

on March 15 you changed to priority of PR 35435 and PR 35441 to P4.
IMHO this is not in line with our current policy:

* PR 35435 is not an error-recovery bug (i.e. we don't issue a valid
  error message before the ICE). So this should rather be P2, I think.
* PR 35441 is a diagnostic bug, in which completely garbled diagnostics
  are emitted. This is a user-visible regression. Richard Guenther
  changed the related bugs PR 35442 and 35443 to P2, which I think is
  the right setting.

Would you mind changing the priorities? Or explain why P4 is the correct
setting after all?

Thanks,
Volker

September entry on mailing list pages broken

[Volker Reichelt]

Hi,

the starting pages for the mailing lists like
http://gcc.gnu.org/ml/gcc  or  http://gcc.gnu.org/ml/gcc-patches
are broken. To be more precise, the entry generated for September is
corrupted: It shows a strange size, and the link points to a wrong location.

  * October, 2007
  * September, 2007 [mbox-formatted archive (/pub/gcc/mail-archives bytes)]
  * August, 2007 [mbox-formatted archive (510.0 Kbytes)]
  * July, 2007 [mbox-formatted archive (783.5 Kbytes)]
  [etc.]

Can anybody fix this?

Regards,
Volker

Semicolons at the end of member function definitions

[Volker Reichelt]

Hi,

I just stumbled over the patch

2007-03-26  Dirk Mueller  [EMAIL PROTECTED]

   * parser.c (cp_parser_member_declaration): Pedwarn
   about stray semicolons after member declarations.

which was approved by Gaby here:
http://gcc.gnu.org/ml/gcc-patches/2007-03/msg01456.html
and made it into the trunk here:
http://gcc.gnu.org/ml/gcc-cvs/2007-03/msg00841.html

It makes

  struct A
  {
 void foo() {};
  }

a hard error with -pedantic.

The 1998 version of the standard (sorry, I don't have the 2003 version
available) contains in [class.mem]:

  member-declaration:
...
function-definition ;opt
...

Therefore, IMHO the patch is wrong and should be reverted.
Or am I missing something?

Regards,
Volker

Bootstrap failure with Objective-C++

[Volker Reichelt]

Hi,

bootstrap with Objective-C++ seems to be broken on mainline (rev. 122792):

/gccbuild/src-4.3/build/./prev-gcc/xgcc -B/gccbuild/src-4.3/build/./prev-gcc/ 
-B/GCC/FARM/gcc-4.3-20070310/i686-pc-linux-gnu/bin/ -c   -O2 -g 
-fomit-frame-pointer -DIN_GCC   -W -Wall -Wwrite-strings -Wstrict-prototypes 
-Wmissing-prototypes -pedantic -Wno-long-long -Wno-variadic-macros 
-Wno-overlength-strings -Wold-style-definition -Wmissing-format-attribute 
-Werror -DOBJCPLUS -I../../gcc/gcc/objc -I../../gcc/gcc/cp -fno-common   
-DHAVE_CONFIG_H -I. -Iobjcp -I../../gcc/gcc -I../../gcc/gcc/objcp 
-I../../gcc/gcc/../include -I../../gcc/gcc/../libcpp/include 
-I/Pakete/GMP/include -I/Pakete/MPFR-2.2.1/include 
-I../../gcc/gcc/../libdecnumber -I../libdecnumber-I. -Iobjcp 
-I../../gcc/gcc -I../../gcc/gcc/objcp -I../../gcc/gcc/../include 
-I../../gcc/gcc/../libcpp/include -I/Pakete/GMP/include 
-I/Pakete/MPFR-2.2.1/include -I../../gcc/gcc/../libdecnumber -I../libdecnumber 
../../gcc/gcc/objc/objc-act.c -o objcp/objcp-act.o
cc1: warnings being treated as errors
../../gcc/gcc/objc/objc-act.c: In function 'lookup_method_in_protocol_list':
../../gcc/gcc/objc/objc-act.c:570: error: comparison is always false due to 
limited range of data type
../../gcc/gcc/objc/objc-act.c: In function 'lookup_protocol_in_reflist':
../../gcc/gcc/objc/objc-act.c:598: error: comparison is always false due to 
limited range of data type
../../gcc/gcc/objc/objc-act.c:605: error: comparison is always false due to 
limited range of data type
../../gcc/gcc/objc/objc-act.c: In function 'generate_protocol_references':
../../gcc/gcc/objc/objc-act.c:4443: error: comparison is always false due to 
limited range of data type

[snip]

../../gcc/gcc/objc/objc-act.c: In function 'objc_lookup_ivar':
../../gcc/gcc/objc/objc-act.c:9428: error: comparison is always false due to 
limited range of data type
../../gcc/gcc/objc/objc-act.c:9440: error: comparison is always false due to 
limited range of data type
make[3]: *** [objcp/objcp-act.o] Error 1
make[3]: *** Waiting for unfinished jobs
rm gcj-dbtool.pod grmiregistry.pod fsf-funding.pod jcf-dump.pod jv-convert.pod 
grmic.pod gcov.pod gcj.pod gfdl.pod cpp.pod gij.pod gpl.pod gc-analyze.pod 
gfortran.pod gcc.pod
make[3]: Leaving directory `/gccbuild/src-4.3/build/gcc'
make[2]: *** [all-stage2-gcc] Error 2
make[2]: Leaving directory `/gccbuild/src-4.3/build'
make[1]: *** [stage2-bubble] Error 2
make[1]: Leaving directory `/gccbuild/src-4.3/build'
make: *** [bootstrap-lean] Error 2

Is anybody else seeing this?

Regards,
Volker

Re: GCC-4.0.4 release status

[Volker Reichelt]

Hi,

 There were over 250 PRs open against GCC-4.0.4.  Almost all of
 them are benign in the sense that we can leave without fixing them
 in GCC-4.0.4 -- many are already fixed in more recent versions.
 I'm now giving attention only to those PRs marked as blocker or
 critical.  I've identified three:

Well, there's another serious (wrong-code) bug which should be fixed IMHO:

PR c++/29106 is a C++ front-end issue.
It has a one-line fix (plus comment) on the 4.1 branch.
Well, actually one should also backport the fix for PR c++/28284 then,
which is a two-liner.

Unfortunately, I didn't find the time to test the patch myself.

Regards,
Volker

Re: GCC-4.0.4 release status

[Volker Reichelt]

Hi,

 | Well, there's another serious (wrong-code) bug which should be fixed
IMHO:
 |
 | PR c++/29106 is a C++ front-end issue.
 | It has a one-line fix (plus comment) on the 4.1 branch.
 | Well, actually one should also backport the fix for PR c++/28284 then,
 | which is a two-liner.

 I was primarily looking at the PRs that marked in the bugzilla
 database blocker or critical.  As there were over 256 PRs open, and
 the idea is to get GCC-4.0.4 out of the door as soon as possible, I'm
 not trying to fix everything; just those that are critical or
 blockers.  This is based on the fact that most distros have moved to
 GCC-4.1.x or higher.  GCC-4.0.x has been since GCC-4.0.0 to contain
 major shortcomings.

Well, the severity status of the bugs is not very well maintained.
Mark e.g. only sets the prioriy field (P1 - P5) of the bugs.
And PR 29106 bug is one of the 37 P1 bugs. And one from three
wrong-code P1 bugs. So this is not like every simple error-recovery
problem.

In addition this is a regression from GCC 4.0.2, i.e. a regression
on the 4.0 branch. Which makes this bug even worse, IMHO.
(This infromation seems to be missing in bugzilla, though.)

Considering how much dispute there is on the mailing list about how
to handle undefined behaviour correctly ;-), it bothers me more that
we ignore one-lines fixes for wrong-code bugs.

Regards,
Volker

Bootstrap failure caused by jvmti additions

[Volker Reichelt]

Hi Tom,

your patch http://gcc.gnu.org/ml/java-patches/2006-q3/msg00264.html
broke bootstrap (at least on x86_64-unknown-linux-gnu):

ranlib .libs/libgij.a
creating libgij.la
./.libs/libgcj.so: undefined reference to `JvNumMethods(java::lang::Class*)'
./.libs/libgcj.so: undefined reference to `JvGetFirstMethod(java::lang::Class*)'
collect2: ld returned 1 exit status
make[5]: *** [jv-convert] Error 1

Regards,
Volker

New testsuite failures with -fprofile-use

[Volker Reichelt]

Hi,

since a couple of days we have the following new failures on mainline:

FAIL: gcc.dg/tree-prof/inliner-1.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/inliner-1.c execution,-fprofile-use
FAIL: gcc.dg/tree-prof/val-prof-1.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/val-prof-1.c execution,-fprofile-use
FAIL: gcc.dg/tree-prof/val-prof-2.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/val-prof-2.c execution,-fprofile-use
FAIL: gcc.dg/tree-prof/val-prof-3.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/val-prof-3.c execution,-fprofile-use
FAIL: gcc.dg/tree-prof/val-prof-4.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/val-prof-4.c execution,-fprofile-use
FAIL: gcc.dg/tree-prof/val-prof-5.c compilation,  -fprofile-use (internal 
compiler error)
UNRESOLVED: gcc.dg/tree-prof/val-prof-5.c execution,-fprofile-use

See for example:
http://gcc.gnu.org/ml/gcc-testresults/2006-07/msg01139.html

The testresults of 2006-07-24 seem to be clean, though.

Here's one of the failures:

  % gcc -O2 -fprofile-generate val-prof-1.c
  % a.out 
  % gcc -O2 -fprofile-use val-prof-1.c
  val-prof-1.c: In function 'main':
  val-prof-1.c:17: internal compiler error: in set_bb_for_stmt, at 
tree-cfg.c:2775
  Please submit a full bug report, [etc.]

Is anybody looking into this?

Regards,
Volker

[gomp] EH problems with OpenMP

[Volker Reichelt]

Hi,

OpenMP is currently more or less unusable with the C++ front-end
because of EH issues (see PR26823). This unfortunate situation
is dragging on for more than two months now and makes further
testing impossible.

Some of the problems were fixed in PR26084, some still remain:
The original (unreduced) testcase from PR26084 still causes an ICE.
This is probably the same problem as in PR26823. We also have a third
PR about an EH problem (PR26913) which has probably the same cause.

Alas there seems to be no activity in that direction since PR26084
was closed. Could somebody of the gomp-maintainers please have a look?

Thanks a lot in advance,
Volker

Re: .cvsignore in libjava/classpath

[Volker Reichelt]

On 20 Feb, Andrew Haley wrote:
 Andrew Pinski writes:

In libjava/classpath there are two .cvsignore files which haven't been
deleted yet:

  native/jni/midi-alsa/.cvsignore
  native/jni/midi-dssi/.cvsignore

Should they go, too?
They are also in GCC 4.1.0 RC1.
   
   They are imported from upstream :).
 
 Yeah.  Don't delete *anything* in libjava/classpath: instead go to
 :ext:cvs.savannah.gnu.org:/sources/classpath.
 
 Andrew.

I didn't want to delete it myself, since I suspected something like this.
Would somebody of the libjava maintainers take care of this?

Thanks,
Volker

.cvsignore in libjava/classpath

[Volker Reichelt]

In libjava/classpath there are two .cvsignore files which haven't been
deleted yet:

  native/jni/midi-alsa/.cvsignore
  native/jni/midi-dssi/.cvsignore

Should they go, too?
They are also in GCC 4.1.0 RC1.

Regards,
Volker

Re: cp/do_poplevel

[Volker Reichelt]

On 25 Jan, Marcin Dalecki wrote:
 The following:
 
 2006-01-23  Volker Reichelt  [EMAIL PROTECTED]
 
   * cp-tree.h (do_poplevel): Remove prototype.
   * semantics.c (do_poplevel): Add prototype.  Make static.
 
 
 Is a plain mistake due to:
 
 ../.././gcc/objcp/objcp-decl.c: In function 'tree_node*  
 objcp_end_compound_stmt(tree_node*, int)':
 ../.././gcc/objcp/objcp-decl.c:114: error: 'do_poplevel' was not  
 declared in this scope
 ../.././gcc/objcp/objcp-decl.c:118: error: 'do_poplevel' was not  
 declared in this scope

Since Objective-C++ isn't built by default I didn't see that.
I just reverted the patch, see
  http://gcc.gnu.org/ml/gcc-cvs/2006-01/msg01012.html
Sorry for the inconvenience!

Regards,
Volker

Re: cp/default_conversion

[Volker Reichelt]

On 25 Jan, Marcin Dalecki wrote:
 The following removal of global default_conversion inside the C++  
 frontend:
 
 2006-01-25  Volker Reichelt  [EMAIL PROTECTED]
 
   (default_conversion): Likewise.
 
 Is junk due to the fact that it gets used for example in rs6000/rs6000.c
 The results in *actual* build failure on Darwin/PowerPC.

I reverted the patch to cure the bootstrap-failure, but there are
some things that make me wonder:

The function default_conversion is used in rs6000/rs6000-c.c.
The first line of the file reads:

  /* Subroutines for the C front end on the POWER and PowerPC architectures.

The file doesn't include cp-tree.h, but only c-tree.h.
But nevertheless the function default_conversion from the C++ frontend
is linked and not the one from the C frontend.

Is that intentional or a bug?
Do we need a langhook to get the right function?

Regards,
Volker

Re: Add revision number to gcc version?

[Volker Reichelt]

 1. contrib/gcc_update creates gcc/REVISION with branch name and
 revision number.
 2. If gcc/REVISION exists, it will be used in gcc/version.c.
 
 With those 2 patches, I got
 
 [EMAIL PROTECTED] gcc]$ ./xgcc --version
 xgcc (GCC) 4.1.0 (gcc-4_1-branch revision 108596) 20051215 (prerelease)

[snip]

 xgcc (GCC) 4.2.0 (trunk revision 108596) 20051215 (experimental)

IMHO we should change the current naming system as little as possible
for those who run scripts to extract the information from the compiler
automatically. (Maybe I'm the only one, but I do.)

Therefore I'd like to suggest to add the new stuff at the end and
with brackets instead of parentheses to make that information easily
extractable as well.

xgcc (GCC) 4.1.0 20051215 (prerelease) [gcc-4_1-branch revision 108596]
xgcc (GCC) 4.2.0 20051215 (experimental) [trunk revision 108596]

Somebody on this thread also suggested to skip the date and only use
the revision number. I don't think that this is a good idea, because

1.) there are scripts out there that rely on the date
(OK, maybe I'm the only one)
2.) if we ever change the revision control system again,
these numbers are probably obsolete
3.) the date carries a lot of information for humans (oh, this snapshot
is already two month old, I should get a new one) without having
to consult a database (for those who only download snapshots).
And this info is available offline, too.

Regards,
Volker

Re: C++ parser: Should we get rid of cp_parser_declarator_id?

[Volker Reichelt]

On  7 Dec, Nathan Sidwell wrote:
 Gabriel Dos Reis wrote:
 
 If we make it static inline, would not we gain the same efficiency
 and preserve the comments and all that?  In general, the methodoly
 seems to have a function for each non-terminal -- following a long
 tradition of recursive descent parser -- and maintaining that
 principle is helpful for code clarity.

Ok. I thought less indirection would make the code more readable,
but you've got a point here.

 There's no need to make it inline.  The optimizers are now smart enough to 
 inline a static function into its only caller.

That's what I thought.
I'll leave the code as is, then.

Regards,
Volker

Renaming build_function_call_expr?

[Volker Reichelt]

Hi,

back in August I removed a call to fold after build_function_call_expr,
since build_function_call_expr already does the folding. In a follow-up
mail Richard Guenther raised the following point
( see http://gcc.gnu.org/ml/gcc-patches/2005-08/msg01466.html ):

 Following precedence and to avoid such issue in future should we rename
 this to fold_build_function_call_expr?  Or build_fold_function_call_expr.

The problem is that there are about 113 occurrences of
build_function_call_expr in the GCC tree. I wonder whether
renaming them would be a too big change with too little benefit.

Would it be more appropriate to just change the comment on top of the
function (in builtins.c, line ~8760) from

  /* Conveniently construct a function call expression.  */

to

  /* Conveniently construct and fold a function call expression.  */

?

Any preferences?

Regards,
Volker

C++ parser: Should we get rid of cp_parser_declarator_id?

[Volker Reichelt]

The C++ parser contains the static function
  cp_parser_declarator_id (cp_parser* parser)
which consists of a lot of comments and a single statement

  return cp_parser_id_expression (parser,
  /*template_keyword_p=*/false,
  /*check_dependency_p=*/false,
  /*template_p=*/NULL,
  /*declarator_p=*/true);

and which has a single caller.

Shouldn't we fold cp_parser_declarator_id into the caller and call
cp_parser_id_expression directly?
But what about the comments then? (Are they still accurate, btw.?)
Or should we leave the function intact just to preserve the comments?

Regards,
Volker

[gomp] Does it make sense to post bug reports already?

[Volker Reichelt]

Hi,

I just wanted to know what's the state of the gomp branch w.r.t
bug reports. Does it make sense to already send bug reports to
you or even add them to bugzilla?

We've got a large C++ application that uses OpenMP and we are really
interested in getting gomp work.

Here's one bug for starters:

  void foo()
  {
  int i;
  #pragma omp parallel for
  for ( i=0; i10; ++i )
  continue;
  }

With the C frontend I get:
gbug.c: In function '__omp_fn.1':
gbug.c:6: error: invalid exit from OpenMP structured block

With the C++ frontend I get:
gbug.cc: In function 'void foo()':
gbug.cc:6: error: continue statement not within loop or switch

Regards,
Volker

rsync access seems to be broken

[Volker Reichelt]

Hi,

since two days I cannot synchronize my gcc repository using rsync
anymore. Nothing happens and after a some time I get the following
error message:

rsync: read error: Connection reset by peer (104)
rsync error: error in rsync protocol data stream (code 12) at io.c(584)

Could somebody please have a look?

Regards,
Volker

Re: rsync access seems to be broken

[Volker Reichelt]

On 12 Oct, Jonathan Larmour wrote:
 Volker Reichelt wrote:
 
 since two days I cannot synchronize my gcc repository using rsync
 anymore. Nothing happens and after a some time I get the following
 error message:
 
 Some stale connections were clogging up the rsync port - there's a 
 connection limit of 16. Nearly all of them were from AMD.
 
 I expunged the dead connections and it should work better now.

Indeed, it's working again.
Thanks!

Btw, I get no response for
ping gcc.gnu.org
Is this intended? Or does this also need fixing?

Regards,
Volker

 Jifl

Re: Merged CVS repository of gcc and old-gcc

[Volker Reichelt]

Ian Lance Taylor wrote in http://gcc.gnu.org/ml/gcc/2005-07/msg00625.html:

 In preparation for the future transition to subversion, I've written
 some code to merge the old-gcc repository into current mainline.  I
 would like to see this merged repository used as the basis for the
 conversion to subversion.  The advantage is that it provides revision
 history back to 1992, when the gcc sources were first put into a
 source code control system.  (At the time, it was RCS.  Before 1992
 the source code control system was emacs numbered backup files.)
 
 Since I just wrote this code, I'd like any feedback that people care
 to give on the correctness and usability of the generated repository.
 People with SSH access to sourceware should be able to access the
 temporary merged repository by doing
 cvs -d :ext:gcc.gnu.org:/pool/ian/repo co gcc

[snip]

 By the way, in case anybody asks, I will not be doing this merge
 before the subversion conversion, because it changes all the CVS
 revision numbers and thus breaks all existing working directories.

What will happen to the (revision number based) hyperlinks to patches
in Bugzilla and the gcc-cvs mailing list archive like the following:

http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reg-stack.c.diff?cvsroot=gccr1=1.188r2=1.189

Will they still point to something useful?
If not, that would render the whole gcc-cvs archive list useless. :-(

Well, this is a question for the cvs to svn conversion in general,
but gets more complicated with your merge of the cvs repositories.

Regards,
Volker

Re: Bootstrap Failure (i686-pc-linux-gnu, --with-arch=pentium4)

[Volker Reichelt]

 Hi,
 
   For two consecutive days, I have been unable to
 build GCC mainline on i686-pc-linux-gnu:

 /home/ranmath/src/gcc/gcc-20050617/gcc/config/i386/i386.c: In function
 'ix86_expand_vector_init':
 /home/ranmath/src/gcc/gcc-20050617/gcc/config/i386/i386.c:17080:
 error: insn does not satisfy its constraints:
 (insn 367 343 378 4
 /home/ranmath/src/gcc/gcc-20050617/gcc/config/i386/i386.c:17047 (set
 (reg:QI 138)
 (const_int 0 [0x0])) 45 {*movqi_1} (nil)
 (expr_list:REG_EQUIV (const_int 0 [0x0])
 (nil)))
 /home/ranmath/src/gcc/gcc-20050617/gcc/config/i386/i386.c:17080:
 internal compiler error: in reload_cse_simplify_operands, at
 postreload.c:391
 Please submit a full bug report,
 with preprocessed source if appropriate.
 See URL:http://gcc.gnu.org/bugs.html for instructions.
 - 8
 -
 
 I build with --with-arch=pentium4 --disable-checking and that
 might explain why no one has apparently reported it yet. 

It has already been reported, it's PR22089 ;-)
http://gcc.gnu.org/PR22089

Nobody patches reload without causing regressions.
Sometimes even looking hard at the code can cause failures. ;-)

 Thanks,
 Ranjit.

Regards,
Volker

C++ function pointer weirdness

[Volker Reichelt]

Yesterday the output of the following program changed
(probably due to the fix for PR19076):

==
template typename T int ref (T){ return 0; }
template typename T int ref (const T)  { return 1; }
template typename T int ref (const volatile T) { return 2; }
template typename T int ref (volatile T)   { return 4; }

template typename T int ptr (T*){ return 0; }
template typename T int ptr (const T*)  { return 8; }
template typename T int ptr (const volatile T*) { return 16; }
template typename T int ptr (volatile T*)   { return 32; }

void foo() {}

int main()
{
return ref(foo) + ptr(foo);
}
==

GCC 2.95.3 - 3.4.0 return 0, GCC 3.4.1 - 3.4.4-20050222 return 2,
and now mainline again returns 0.

So the question is: What is the correct return value?

Btw, we really should have this in the testsuite.

In any case, we have a wrong-code regression here, either on the
3.4 branch or on mainline. But before I open a PR I'd like to sort
out which is the correct behavior.

When the result changed in 3.4.1 I bugged Nathan (who caused this
change) about it, and he claimed that '2' is the correct result.
Intel's compiler indeed returns 2.

Regards,
Volker