This is an archive of the discontinued LLVM Phabricator instance.

[clang] Include type specifiers in typo correction when checking isCXXDeclarationSpecifiers.
ClosedPublic

Authored by hokein on Jul 2 2020, 12:17 AM.

Details

Summary

Diff Detail

Event Timeline

hokein created this revision.Jul 2 2020, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 12:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein edited the summary of this revision. (Show Details)Jul 2 2020, 3:06 AM
hokein added a reviewer: sammccall.
sammccall accepted this revision.Jul 13 2020, 2:09 AM
sammccall added inline comments.
clang/test/SemaCXX/typo-correction.cpp
614

are these one testcase or two?
If they're independent, please use different identifiers to avoid confusion

623

"resolve" and 5 lines below

This revision is now accepted and ready to land.Jul 13 2020, 2:09 AM
hokein updated this revision to Diff 277363.Jul 13 2020, 3:04 AM
hokein marked 2 inline comments as done.

rebase and address comments.

This revision was automatically updated to reflect the committed changes.
sammccall added inline comments.Jul 13 2020, 3:37 AM
clang/test/SemaCXX/typo-correction.cpp
614

I'm not sure this comment was addressed?
both testIncludeTypeInTemplateArgument and testNoCrashOnNullNNSTypoCorrection use the name "AddObservation", which seems like something very specific but in fact these tests are unrelated.

aganea added a subscriber: aganea.Nov 11 2020, 9:45 AM

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

thanks for the testcase. I'm not sure the patch is the root cause -- from the Bugzilla thread, the issue was reported in April which is prior to this patch.

btw, the issue seems to be gone on the trunk, I don't reproduce them with clang building from the trunk, I guess it might be fixed by some patches after llvm-11.

We're seeing the same issue mentionned in PR45498, but with the repro below. git bisect points to this patch. Could anyone please possibly confirm? Simply build clang with -DLLVM_ENABLE_ASSERTIONS=ON.

// compile with: clang-cl /c a.cpp
template < a > struct b;
template < bool a, class > using c = b< a >;
template < class > using d = void ;
template < class, class, class = void >
bool e template < class f, class g >
bool e< f, g, d< decltype(h(g())) > > template < class f, class g >
void i(f, g ) {
  e< f, g >
}
template < class _BidIt2, c< e< _BidIt, _BidIt2 >, int > = 0 >
void h(_BidIt2) short do_tolower__Last {
  i(do_tolower__First, do_tolower__Last)
}

thanks for the testcase. I'm not sure the patch is the root cause -- from the Bugzilla thread, the issue was reported in April which is prior to this patch.

btw, the issue seems to be gone on the trunk, I don't reproduce them with clang building from the trunk, I guess it might be fixed by some patches after llvm-11.

Thanks for getting back! Just for the record, bisecting between the llvmorg-12-init git tag and rG637f19c36b3 leads to this fix: D87853.

commit cffb0dd54d41d8e249d2009467c4beb5b681ba26
Author: Bruno Cardoso Lopes <bruno.cardoso@gmail.com>
Date:   Mon Oct 12 15:58:52 2020 -0700

    [SemaTemplate] Stop passing insertion position around during VarTemplate instantiation

    They can get stale at use time because of updates from other recursive
    specializations. Instead, rely on the existence of previous declarations to add
    the specialization.

    Differential Revision: https://reviews.llvm.org/D87853

I was able to cherry-pick it cleanly on release/11.x and that solves our issue.