aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5328
+ if (!IdentLoc || !IdentLoc->Ident) {
+ // Try to locate the argument directly
+ SourceLocation Loc = AL.getLoc();
----------------
Comments should be grammatical including punctuation (elsewhere as well).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5340
+ // Verify that the identifier is a valid decl in the C decl namespace
+ LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+ SourceLocation(),
----------------
`lookupResult` -> `Result` (or something else that matches the usual naming
conventions).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+ D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
----------------
Shouldn't we also be validating that what we found is an NSString that has the
correct properties? (That doesn't seem like it should be a change which risks
breaking code based on what I understood from Doug's description.)
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+ S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+ << identLoc->Ident;
+ return;
----------------
MForster wrote:
> aaron.ballman wrote:
> > We're doing this lookup in the context of where the attribute is being
> > used, but then creating the attribute with only the identifier and not the
> > result of that lookup. This makes me a bit worried that when something goes
> > to resolve that identifier to a variable later, it may get a different
> > result because the lookup context will be different. Do you need to store
> > the VarDecl on the semantic attribute, rather than the identifier?
> >We're doing this lookup in the context of where the attribute is being used,
> >but then creating the attribute with only the identifier and not the result
> >of that lookup. This makes me a bit worried that when something goes to
> >resolve that identifier to a variable later, it may get a different result
> >because the lookup context will be different. Do you need to store the
> >VarDecl on the semantic attribute, rather than the identifier?
>
>
> When this gets imported into Swift, only the name of the identifier gets
> used. I'm not quite sure why this was defined like this. This is another
> thing where I would hope that @gribozavr2 or @milseman know more...
Based on the answer from @doug.gregor, I think this should be adding the result
of the lookup to the semantic attribute and not the identifier (the identifier
may not be unique, the VarDecl must be unique though).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84005/new/
https://reviews.llvm.org/D84005
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
