[cfe-commits] [PATCH] Allow redeclaring a function in an inner scope with different default arguments

[James Molloy]

Hi John,

Thanks for the quick review!

> I would prefer to only compute this when there's actually a default argument
> on the old declaration.  It should be easy enough to just pass down the scope
> of the new declaration.

Totally sensible and doable.

> This doesn't seem to be right.  We should be ignoring the default argument
> in other scopes regardless of whether the new declaration has a default
> argument.

You're right, I'm surprised I didn't see this myself.

> Also, don't we need to look for previous declarations that *are* in the same
> scope and make sure we use their default arguments?  I'm thinking of
> something like:

There are already testcases for that behaviour, and they still pass under this patch.

I'll update the patch on Monday.

Cheers!

James
________________________________________
From: John McCall [rjmccall at apple.com]
Sent: 09 March 2012 18:09
To: James Molloy
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: [cfe-commits] [PATCH] Allow redeclaring a function in an inner     scope with different default arguments

On Mar 9, 2012, at 4:02 AM, James Molloy wrote:
> The following code is legal according to the c++ standard:
>
> void f() {
>  int g(int a, int b=4);
>  {
>    int g(int a, int b=5);
>  }
> }
>
> Because "[snip] That is, declarations in inner scopes do not acquire default
> arguments from declarations in outer scopes, and vice versa."
>
> Clang currently quotes this part of the standard in a comment, but does not
> adhere to it. The attached patch fixes this so the above code compiles
> without errors.
>
> This involves passing information about whether the new and old declarations
> are in the same Scope when merging them.

+      bool IsInSameCXXScope = true;
+      if (getLangOptions().CPlusPlus && S)
+        IsInSameCXXScope = isDeclInScope(OldDecl, NewFD->getDeclContext(), S);
+

I would prefer to only compute this when there's actually a default argument
on the old declaration.  It should be easy enough to just pass down the scope
of the new declaration.

-    if (OldParam->hasDefaultArg() && NewParam->hasDefaultArg()) {
+    bool OldParamHasDfl = OldParam->hasDefaultArg();
+    bool NewParamHasDfl = NewParam->hasDefaultArg();
+
+    if (!IsInSameCXXScope)
+      // Ignore default parameters of old decl if they are not in
+      // the same scope.
+      OldParamHasDfl = false;

This doesn't seem to be right.  We should be ignoring the default argument
in other scopes regardless of whether the new declaration has a default
argument.

Also, don't we need to look for previous declarations that *are* in the same
scope and make sure we use their default arguments?  I'm thinking of
something like:

  void test() {
    void foo(int, int = 7);
    { void foo(int, int = 5); }
    void foo(int = 5, int); // valid, with the second argument defaulting to 7
  }

John.



-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.