[PATCH v2 5/6] Add a flag to BuildDeclarationNameExpr to not reject invalid decls.

[David Blaikie]

On Thu, Nov 13, 2014 at 11:23 AM, Kaelyn Takata <rikka at google.com> wrote:

>
>
> On Wed, Nov 12, 2014 at 3:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Wed, Nov 5, 2014 at 1:06 PM, Kaelyn Takata <rikka at google.com> wrote:
>>
>>>
>>>
>>> On Tue, Nov 4, 2014 at 4:36 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> It's helpful to either use an enum (which always seems a bit silly) or
>>>> a comment (more realistic, especially since you only have to comment that
>>>> single caller in attemptRecovery) to describe what the boolean value is for
>>>> ("/*AcceptInvalidDecl*/ true").
>>>>
>>>
>>> I've added comments for both boolean values in the call to
>>> SemaRef.BuildDeclarationNameExpr from attemptRecovery.
>>>
>>>>
>>>> What's this flag needed/used for?
>>>>
>>>
>>> The flag is needed to get a valid DeclRefExpr during error recovery even
>>> when the Decl isn't marked as valid.
>>>
>>
>> Why does this need to be a flag? (what bad things happen if we always
>> accept invalid decls?)
>>
>
> Setting the flag to always be true breaks tests in
> CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp, SemaCXX/dependent-auto.cpp, SemaCXX/lambda-expressions.cpp,
> and SemaObjCXX/instantiate-stmt.mm. It also causes the assertion failure
> "cannot request the size of an undeduced or dependent auto type"
> in SemaCXX/lambda-expressions.cpp.
>

OK - I'll sign off on this. Commit whenever it fits/suits.

(though it is hard for me to follow the motivation/need for things like
this when presented in isolation for review (yes, as part of a patch set,
but it's still hard to piece the context together from a series of patches)
- not sure if there's a great answer here. Perhaps not)

- David


>
>
>>
>>> For example, with the full patch set, not having the flag causes the
>>> last error of test/SemaCXX/conversion-function.cpp (at line 419) to fail to
>>> be typo-corrected. With the flag or without this patchset:
>>>
>>> ~/llvm/tools/clang/test/SemaCXX/conversion-function.cpp:419:18: error:
>>> no member named 'e' in 'PR18234::A'; did you mean simply 'e'?
>>>   bool k1 = e == A::e; // expected-error {{no member named 'e'}}
>>>                  ^~~~
>>>                  e
>>> ~/llvm/tools/clang/test/SemaCXX/conversion-function.cpp:418:8: note: 'e'
>>> declared here
>>>   A::E e = a; // expected-note {{here}}
>>>        ^
>>> 4 warnings and 26 errors generated.
>>>
>>> With this patchset but without this flag:
>>>
>>> ~/llvm/tools/clang/test/SemaCXX/conversion-function.cpp:419:21: error:
>>> no member named 'e' in 'PR18234::A'
>>>   bool k1 = e == A::e; // expected-error {{no member named 'e'}}
>>>                  ~~~^
>>> 4 warnings and 26 errors generated.
>>>
>>>
>>>> On Wed, Oct 29, 2014 at 12:49 PM, Kaelyn Takata <rikka at google.com>
>>>> wrote:
>>>>
>>>>> ---
>>>>>  include/clang/Sema/Sema.h |  6 ++++--
>>>>>  lib/Sema/SemaExpr.cpp     | 15 ++++++++-------
>>>>>  lib/Sema/SemaExprCXX.cpp  |  2 +-
>>>>>  3 files changed, 13 insertions(+), 10 deletions(-)
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141117/84da820d/attachment.html>