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 [[email protected]] Sent: 09 March 2012 18:09 To: James Molloy Cc: [email protected] 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
