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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
