public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] c++, v2: Diagnose [basic.scope.block]/2 violations even for block externs [PR52953]
Date: Fri, 1 Sep 2023 15:34:18 +0200	[thread overview]
Message-ID: <ZPHoWgZhqZFQP8cX@tucnak> (raw)
In-Reply-To: <495f5797-fbb9-e9f6-faa4-af994782edd5@redhat.com>

On Thu, Aug 31, 2023 at 05:46:28PM -0400, Jason Merrill wrote:
> I've suggested this to Core.

Thanks.

> > So, I'm not really sure what to do.  Intuitively the patch seems right
> > because even block externs redeclare stuff and change meaning of the
> > identifiers and void foo () { int i; extern int i (int); } is rejected
> > by all compilers.
> 
> I think this direction makes sense, though we might pedwarn on these rather
> than error to reduce possible breakage.

It wasn't clear to me whether you want to make those pedwarns just for the
DECL_EXTERNAL cases, ones that actually changed, or all others as well
(which were errors or permerrors depending on the case).
I've implemented the former, kept existing behavior of !DECL_EXTERNAL.

> > 2023-08-31  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/52953
> > 	* name-lookup.cc (check_local_shadow): Defer punting on
> > 	DECL_EXTERNAL (decl) from the start of function to right before
> > 	the -Wshadow* checks.
> 
> Don't we want to consider externs for the -Wshadow* checks as well?

I think that is a good idea (though dunno how much it will trigger in
real-world), but there is one case I've excluded, the global variable
shadowing case, because warning that
int z;
void foo () { extern int z; z = 1; }
shadows the global var would be incorrect, it is the same var.
It is true that
int y; namespace N { void bar () { extern int y; y = 1; } }
shadows ::y but it is unclear how to differentiate those two cases with
the information we have at check_local_shadow time.

I've also found one spot which wasn't using auto_diagnostic_group d;
on a pair of error_at/inform.

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

2023-09-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/52953
	* name-lookup.cc (check_local_shadow): Don't punt early for
	DECL_EXTERNAL decls, instead just disable the shadowing of namespace
	decls check for those and emit a pedwarn rather than error_at for
	those.  Add missing auto_diagnostic_group.  Formatting fix.

	* g++.dg/diagnostic/redeclaration-4.C: New test.
	* g++.dg/diagnostic/redeclaration-5.C: New test.
	* g++.dg/warn/Wshadow-19.C: New test.

--- gcc/cp/name-lookup.cc.jj	2023-09-01 10:21:03.658118594 +0200
+++ gcc/cp/name-lookup.cc	2023-09-01 11:30:10.868516494 +0200
@@ -3096,10 +3096,6 @@ check_local_shadow (tree decl)
   if (TREE_CODE (decl) == PARM_DECL && !DECL_CONTEXT (decl))
     return;
 
-  /* External decls are something else.  */
-  if (DECL_EXTERNAL (decl))
-    return;
-
   tree old = NULL_TREE;
   cp_binding_level *old_scope = NULL;
   if (cxx_binding *binding = outer_binding (DECL_NAME (decl), NULL, true))
@@ -3130,11 +3126,9 @@ check_local_shadow (tree decl)
 	      && DECL_CONTEXT (old) == lambda_function (current_lambda_expr ())
 	      && TREE_CODE (old) == PARM_DECL
 	      && DECL_NAME (decl) != this_identifier)
-	    {
-	      error_at (DECL_SOURCE_LOCATION (old),
-			"lambda parameter %qD "
-			"previously declared as a capture", old);
-	    }
+	    error_at (DECL_SOURCE_LOCATION (old),
+		      "lambda parameter %qD "
+		      "previously declared as a capture", old);
 	  return;
 	}
       /* Don't complain if it's from an enclosing function.  */
@@ -3156,10 +3150,18 @@ check_local_shadow (tree decl)
 	     in the outermost block of the function definition.  */
 	  if (b->kind == sk_function_parms)
 	    {
-	      error_at (DECL_SOURCE_LOCATION (decl),
-			"declaration of %q#D shadows a parameter", decl);
-	      inform (DECL_SOURCE_LOCATION (old),
-		      "%q#D previously declared here", old);
+	      auto_diagnostic_group d;
+	      bool emit = true;
+	      if (DECL_EXTERNAL (decl))
+		emit = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
+				"declaration of %q#D shadows a parameter",
+				decl);
+	      else
+		error_at (DECL_SOURCE_LOCATION (decl),
+			  "declaration of %q#D shadows a parameter", decl);
+	      if (emit)
+		inform (DECL_SOURCE_LOCATION (old),
+			"%q#D previously declared here", old);
 	      return;
 	    }
 	}
@@ -3185,10 +3187,16 @@ check_local_shadow (tree decl)
 	       && (old_scope->kind == sk_cond || old_scope->kind == sk_for))
 	{
 	  auto_diagnostic_group d;
-	  error_at (DECL_SOURCE_LOCATION (decl),
-		    "redeclaration of %q#D", decl);
-	  inform (DECL_SOURCE_LOCATION (old),
-		  "%q#D previously declared here", old);
+	  bool emit = true;
+	  if (DECL_EXTERNAL (decl))
+	    emit = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
+			    "redeclaration of %q#D", decl);
+	  else
+	    error_at (DECL_SOURCE_LOCATION (decl),
+		      "redeclaration of %q#D", decl);
+	  if (emit)
+	    inform (DECL_SOURCE_LOCATION (old),
+		    "%q#D previously declared here", old);
 	  return;
 	}
       /* C++11:
@@ -3314,6 +3322,7 @@ check_local_shadow (tree decl)
 	  || (TREE_CODE (old) == TYPE_DECL
 	      && (!DECL_ARTIFICIAL (old)
 		  || TREE_CODE (decl) == TYPE_DECL)))
+      && !DECL_EXTERNAL (decl)
       && !instantiating_current_function_p ()
       && !warning_suppressed_p (decl, OPT_Wshadow))
     /* XXX shadow warnings in outer-more namespaces */
--- gcc/testsuite/g++.dg/diagnostic/redeclaration-4.C.jj	2023-09-01 10:46:15.646025458 +0200
+++ gcc/testsuite/g++.dg/diagnostic/redeclaration-4.C	2023-09-01 10:46:15.646025458 +0200
@@ -0,0 +1,167 @@
+// PR c++/52953
+// { dg-do compile }
+// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
+
+void
+foo (int x)				// { dg-message "'int x' previously declared here" }
+{
+  extern int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+}
+
+void
+bar (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+  extern int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+}
+catch (...)
+{
+}
+
+volatile int v;
+
+void
+baz ()
+{
+#if __cplusplus >= 201103L
+  auto f = [] (int x) { extern int x; };// { dg-error "declaration of 'int x' shadows a parameter" "" { target c++11 } }
+					// { dg-message "'int x' previously declared here" "" { target c++11 } .-1 }
+#endif
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      extern int x;			// { dg-error "redeclaration of 'int x'" }
+    }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    {
+      extern int x;			// { dg-error "redeclaration of 'int x'" }
+    }
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    extern int x;			// { dg-error "redeclaration of 'int x'" }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    extern int x;			// { dg-error "redeclaration of 'int x'" }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      extern int x;			// { dg-error "redeclaration of 'int x'" }
+    default:;
+    }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    extern int x;			// { dg-error "redeclaration of 'int x'" }
+  while (int x = v)
+    {
+      extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
+    }
+  while (int x = v)
+    extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    {
+      extern int x;			// { dg-error "redeclaration of 'int x'" }
+    }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    extern int x;			// { dg-error "redeclaration of 'int x'" }
+  for (; int x = v; )
+    {
+      extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
+    }
+  for (; int x = v; )
+    extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
+  try
+    {
+    }
+  catch (int x)				// { dg-message "'int x' previously declared here" }
+    {
+      extern int x;			// { dg-error "redeclaration of 'int x'" }
+    }
+}
+
+void
+corge (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (...)
+{
+  extern int x;				// { dg-error "redeclaration of 'int x'" }
+}
+
+void
+fred (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (int)
+{
+}
+catch (long)
+{
+  extern int x;				// { dg-error "redeclaration of 'int x'" }
+}
+
+void
+garply (int x)
+{
+  try
+    {
+      extern int x;
+    }
+  catch (...)
+    {
+      extern int x;
+    }
+}
+
+struct S
+{
+  S (int x)				// { dg-message "'int x' previously declared here" }
+  try : s (x)
+  {
+    extern int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+  }
+  catch (...)
+  {
+  }
+  int s;
+};
+
+struct T
+{
+  T (int x)				// { dg-message "'int x' previously declared here" }
+  try : t (x)
+  {
+  }
+  catch (...)
+  {
+    extern int x;				// { dg-error "redeclaration of 'int x'" }
+  }
+  int t;
+};
+
+struct U
+{
+  U (int x) : u (x)
+  {
+    try
+    {
+      extern int x;
+    }
+    catch (...)
+    {
+      extern int x;
+    }
+  }
+  int u;
+};
+
+struct V
+{
+  V (int x) : v (x)
+  {
+    {
+      extern int x;
+    }
+  }
+  int v;
+};
--- gcc/testsuite/g++.dg/diagnostic/redeclaration-5.C.jj	2023-09-01 10:46:15.646025458 +0200
+++ gcc/testsuite/g++.dg/diagnostic/redeclaration-5.C	2023-09-01 10:46:15.646025458 +0200
@@ -0,0 +1,167 @@
+// PR c++/52953
+// { dg-do compile }
+// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
+
+void
+foo (int x)				// { dg-message "'int x' previously declared here" }
+{
+  extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
+}
+
+void
+bar (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+  extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
+}
+catch (...)
+{
+}
+
+volatile int v;
+
+void
+baz ()
+{
+#if __cplusplus >= 201103L
+  auto f = [] (int x) { extern int x (int); };// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" "" { target c++11 } }
+					// { dg-message "'int x' previously declared here" "" { target c++11 } .-1 }
+#endif
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+    }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    {
+      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+    }
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+    default:;
+    }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+  while (int x = v)
+    {
+      extern int x (int);		// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
+    }
+  while (int x = v)
+    extern int x (int);			// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    {
+      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+    }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+  for (; int x = v; )
+    {
+      extern int x (int);		// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
+    }
+  for (; int x = v; )
+    extern int x (int);			// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
+  try
+    {
+    }
+  catch (int x)				// { dg-message "'int x' previously declared here" }
+    {
+      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+    }
+}
+
+void
+corge (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (...)
+{
+  extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+}
+
+void
+fred (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (int)
+{
+}
+catch (long)
+{
+  extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+}
+
+void
+garply (int x)
+{
+  try
+    {
+      extern int x (int);
+    }
+  catch (...)
+    {
+      extern int x (int);
+    }
+}
+
+struct S
+{
+  S (int x)				// { dg-message "'int x' previously declared here" }
+  try : s (x)
+  {
+    extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
+  }
+  catch (...)
+  {
+  }
+  int s;
+};
+
+struct T
+{
+  T (int x)				// { dg-message "'int x' previously declared here" }
+  try : t (x)
+  {
+  }
+  catch (...)
+  {
+    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
+  }
+  int t;
+};
+
+struct U
+{
+  U (int x) : u (x)
+  {
+    try
+    {
+      extern int x (int);
+    }
+    catch (...)
+    {
+      extern int x (int);
+    }
+  }
+  int u;
+};
+
+struct V
+{
+  V (int x) : v (x)
+  {
+    {
+      extern int x (int);
+    }
+  }
+  int v;
+};
--- gcc/testsuite/g++.dg/warn/Wshadow-19.C.jj	2023-09-01 11:35:21.092200057 +0200
+++ gcc/testsuite/g++.dg/warn/Wshadow-19.C	2023-09-01 11:37:15.997598483 +0200
@@ -0,0 +1,27 @@
+// { dg-do compile }
+// { dg-options "-Wshadow" }
+
+void
+foo (int x)
+{
+  int y = 1;
+  {
+    extern int x;				// { dg-warning "declaration of 'int x' shadows a parameter" }
+    extern int y;				// { dg-warning "declaration of 'y' shadows a previous local" }
+  }
+#if __cplusplus >= 201102L
+  auto fn = [x] () { extern int x; return 0; };	// { dg-warning "declaration of 'x' shadows a lambda capture" "" { target c++11 } }
+#endif
+}
+
+int z;
+
+struct S
+{
+  int x;
+  void foo ()
+  {
+    extern int x;				// { dg-warning "declaration of 'x' shadows a member of 'S'" }
+    extern int z;
+  }
+};


	Jakub


  reply	other threads:[~2023-09-01 13:34 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-31  8:08 [RFC PATCH] c++: " Jakub Jelinek
2023-08-31 21:46 ` Jason Merrill
2023-09-01 13:34   ` Jakub Jelinek [this message]
2023-09-05 14:00     ` [PATCH] c++, v2: " Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZPHoWgZhqZFQP8cX@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).