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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.


================
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();
----------------
aaron.ballman wrote:
> 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?
I wanted to check for two different issues when the field isn't found:

1. The field is outside of the struct, or
2. The field isn't found (or is misspelled).

If I do your version, that distinction goes away.

But if you want me to work on this more, would you mind if I did it in a follow-up commit? I'm not at all happy about adding yet another parameter to a method that already has a million parameters. It would be nice if that was cleaned up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381