On Jun 7, 2011, at 10:00 AM, Kaelyn Uhrain wrote:
>
> On Mon, Jun 6, 2011 at 9:23 PM, Douglas Gregor <[email protected]> wrote:
>
> On Jun 6, 2011, at 6:13 PM, Kaelyn Uhrain wrote:
>
>>
>> + // Only perform the qualified lookups for C++
>> + // FIXME: this breaks
>> test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
>> + if (getLangOptions().CPlusPlus) {
>> + TmpRes.suppressDiagnostics();
>> + for (llvm::SmallPtrSet<IdentifierInfo*,
>> + 16>::iterator QRI = QualifiedResults.begin(),
>> + QRIEnd = QualifiedResults.end();
>> + QRI != QRIEnd; ++QRI) {
>> + for (llvm::SmallPtrSet<NamespaceDecl*,
>> + 16>::iterator KNI = KnownNamespaces.begin(),
>> + KNIEnd = KnownNamespaces.end();
>> + KNI != KNIEnd; ++KNI) {
>> + NameSpecifierAndSize NSS;
>> + DeclContext *Ctx = dyn_cast<DeclContext>(*KNI);
>> + TmpRes.clear();
>> + TmpRes.setLookupName(*QRI);
>> + if (!LookupQualifiedName(TmpRes, Ctx)) continue;
>> +
>> + switch (TmpRes.getResultKind()) {
>> + case LookupResult::Found:
>> + case LookupResult::FoundOverloaded:
>> + case LookupResult::FoundUnresolvedValue:
>> + if (SpecifierMap.find(Ctx) != SpecifierMap.end()) {
>> + NSS = SpecifierMap[Ctx];
>> + } else {
>> + NSS = BuildSpecifier(Context, CurContextChain, Ctx);
>> + SpecifierMap[Ctx] = NSS;
>> + }
>> + Consumer.addName((*QRI)->getName(),
>> TmpRes.getAsSingle<NamedDecl>(),
>> + ED + NSS.second, NSS.first);
>> + break;
>> + case LookupResult::NotFound:
>> + case LookupResult::NotFoundInCurrentInstantiation:
>> + case LookupResult::Ambiguous:
>> + break;
>> + }
>> + }
>> }
>> }
>>
>> I suggest always calling TmpRes.suppressDiagnostics() in the ambiguous case,
>> otherwise you may end up getting ambiguity warnings. I don't know if that's
>> the p4.cpp issue or not.
>>
>> Its not necessary since TmpRes.suppressDiagnostics() was called just before
>> the beginning of the outer loop. And to make sure nothing sneaky is
>> happening, I added an assertion that diagnostics are still suppressed to the
>> ambiguous case, and at least for the test suite it was never triggered. I
>> think the p4.cpp issue is related to the typo correction not (yet) being
>> smart enough regarding argument dependent lookup.
>
> Each time the inner loop calls LookupQualifiedName, it's possible that we
> could end up with an ambiguity. That ambiguity needs to be suppressed, or it
> will be reported later. Perhaps there's more suppression later, but I'd feel
> a bit more comfortable if we always did it after the switch.
>
> My point was that suppressDiagnostics just sets a flag which, as far as I can
> tell is not modified by LookupQualifiedName or any of the other code within
> the inner loop. Unless I'm missing something (and some of the places where
> I've seen suppressDiagnostics() called miss the same thing), calling
> TmpRes.suppressDiagnostics() just before the loop starts will suppress
> diagnostics from anything in the loop--and without assigning the same value
> to an otherwise unchanged variable with every iteration.
I see it it now. Thanks!
>
>>
>> This is looking great, and we're getting close to the point where I'd like
>> to put it into the tree! The major issues that I think need to be resolved
>> before we can commit are:
>> - Getting the same behavior when using PCH and when not using PCH (the
>> serialization issue mentioned at the top)
>> - Testsuite failures
>>
>> As I said, the PCH matter requires a bit more learning and experimenting on
>> my part since I've not done anything with them before. Fixing the test suite
>> failures are a given for me, and have been a requirement for submitting the
>> patch for inclusion into the tree. ;)
>
> I can take PCH off your todo list, since it'll be relatively quick for me to
> add that part.
>
> ^.^ Any adjustments I should make to the data structures to facilitate PCH
> serialization and access?
No, I'll deal with it.
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
