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

[James Molloy]

Hi John,

Attached is a slightly modified version of the patch.

> 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.

I have done this.

> 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.

While I originally agreed with you on this, I was at home, didn't have the
patch available and couldn't see that actually the patch *does* actually do
the right thing. I have added an extra testcase however as this behaviour is
not currently tested. See i() in SemaCXX/default1.cpp.

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

Again, this wasn't tested but does do the right thing. I've added a testcase
into CodeGenCXX/default-arguments.cpp based on the snippet you posted.

Is this OK now?

Cheers,

James

-----Original Message-----
From: John McCall [mailto:rjmccall at apple.com] 
Sent: 09 March 2012 18:10
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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cxx-inner-scope-default-args.v2.diff
Type: application/octet-stream
Size: 6848 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120312/761600aa/attachment.obj>