[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148381#4652056 <https://reviews.llvm.org/D148381#4652056>, @void wrote:

> FINALLY! found out how to do suggestions for typos.

<3 Thank you for your perseverance!



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8420-8428
+    if (Result.getResultKind() == LookupResult::Found) {
+      SourceRange SR = CBA->getCountedByFieldLoc();
+      Diag(SR.getBegin(),
+           diag::err_flexible_array_counted_by_attr_field_not_found_in_struct)
+          << CBA->getCountedByField() << SR;
+
+      SR = Result.getAsSingle<NamedDecl>()->getSourceRange();
----------------
The logic here still seems incorrect. I was expecting the code to look more like this:
```
bool Sema::CheckCountedByAttr(Scope *S, const FieldDecl *FD) {
  const RecordDecl *RD = FD->getParent();
  const auto *CBA = FD->getAttr<CountedByAttr>();
  const IdentifierInfo *FieldName = CBA->getCountedByField();
  DeclarationNameInfo NameInfo(FieldName,
                               CBA->getCountedByFieldLoc().getBegin());
  LookupResult Result(*this, NameInfo, Sema::LookupMemberName);

  LookupName(Result, S);
  if (Result.empty()) {
    CXXScopeSpec SS;
    DeclFilterCCC<FieldDecl> Filter(const_cast<IdentifierInfo *>(FieldName));
    if (DiagnoseEmptyLookup(S, SS, Result, Filter, nullptr, std::nullopt,
                            const_cast<DeclContext *>(FD->getDeclContext())))
      return true;
  }

  const FieldDecl *Field = Result.getAsSingle<FieldDecl>();
  LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
      Context.getLangOpts().getStrictFlexArraysLevel();
  ...
```
I tested this locally on code like:
```
struct not_a_fam {
  int foo;
  int fam[] __attribute__((counted_by(fob)));
};
```
and get a diagnostic like: 
```
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:3:39: error: use of undeclared identifier 'fob'; did you
      mean 'foo'?
    3 |   int fam[] __attribute__((counted_by(fob)));
      |                                       ^~~
      |                                       foo
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:7: note: 'foo' declared here
    2 |   int foo;
      |       ^
1 error generated.
```
Note, I had to add a constructor to `DeclFilterCCC` to expose the base class constructor, and modify `DiagnoseEmptyLookup()` like this:
```
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 2ed31a90c5dc..3c4ade391a5e 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -2458,7 +2458,8 @@ bool Sema::DiagnoseDependentMemberLookup(const LookupResult &R) {
 bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
                                CorrectionCandidateCallback &CCC,
                                TemplateArgumentListInfo *ExplicitTemplateArgs,
-                               ArrayRef<Expr *> Args, TypoExpr **Out) {
+                               ArrayRef<Expr *> Args, DeclContext *LookupCtx,
+                               TypoExpr **Out) {
   DeclarationName Name = R.getLookupName();

   unsigned diagnostic = diag::err_undeclared_var_use;
@@ -2474,7 +2475,9 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
   // unqualified lookup.  This is useful when (for example) the
   // original lookup would not have found something because it was a
   // dependent name.
-  DeclContext *DC = SS.isEmpty() ? CurContext : nullptr;
+  DeclContext *DC =
+      LookupCtx ? LookupCtx : (SS.isEmpty() ? CurContext : nullptr);
+  DeclContext *OrigLookupCtx = DC;
   while (DC) {
     if (isa<CXXRecordDecl>(DC)) {
       LookupQualifiedName(R, DC);
@@ -2517,12 +2520,12 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
           emitEmptyLookupTypoDiagnostic(TC, *this, SS, Name, TypoLoc, Args,
                                         diagnostic, diagnostic_suggest);
         },
-        nullptr, CTK_ErrorRecovery);
+        nullptr, CTK_ErrorRecovery, OrigLookupCtx);
     if (*Out)
       return true;
-  } else if (S &&
-             (Corrected = CorrectTypo(R.getLookupNameInfo(), R.getLookupKind(),
-                                      S, &SS, CCC, CTK_ErrorRecovery))) {
+  } else if (S && (Corrected = CorrectTypo(R.getLookupNameInfo(),
+                                           R.getLookupKind(), S, &SS, CCC,
+                                           CTK_ErrorRecovery, OrigLookupCtx))) {
     std::string CorrectedStr(Corrected.getAsString(getLangOpts()));
     bool DroppedSpecifier =
         Corrected.WillReplaceSpecifier() && Name.getAsString() == CorrectedStr;
@@ -2812,7 +2815,7 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
     // a template name, but we happen to have always already looked up the name
     // before we get here if it must be a template name.
     if (DiagnoseEmptyLookup(S, SS, R, CCC ? *CCC : DefaultValidator, nullptr,
-                            std::nullopt, &TE)) {
+                            std::nullopt, nullptr, &TE)) {
       if (TE && KeywordReplacement) {
         auto &State = getTypoExprState(TE);
         auto BestTC = State.Consumer->getNextCorrection();
```
Can you give something like this a shot and see how well it works for you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148381/new/

https://reviews.llvm.org/D148381