[PATCH] D148381: [Clang] Implement the 'counted_by' attribute
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 3 06:31:47 PDT 2023
aaron.ballman 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();
----------------
void wrote:
> 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.
> 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.
Correct, my version is only looking for the field as a member name because we never want it to find a variable outside the scope of the structure. We could augment it to do what you want though:
```
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()) {
// Look for the name outside of the scope of the structure so we can issue a different
// diagnostic in that case.
LookupResult BackupResult(*this, NameInfo, Sema::LookupOrdinaryName);
LookupName(BackupResult, S);
if (!BackupResult.empty()) {
return Diag(Loc, diag::whatever);
} else {
// Otherwise, diagnose the empty lookup.
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();
...
```
> 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.
I don't insist on doing it as part of this patch but I do strongly prefer doing it now. Waiting runs the risk of not actually doing that work, but also, I worry that the lookups will be subtly different and we might risk breaking code by changing it later. That said, if you mean "(almost) immediately after landing this" kind of follow-up, I'm not worried about the breakage and that's fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148381/new/
https://reviews.llvm.org/D148381
More information about the cfe-commits
mailing list