Yes, I mean to do it as a direct follow-up. 😊
-bw
On Tue, Oct 3, 2023, 6:31 AM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:
> 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
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
