aaron.ballman added inline comments.
================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350 + + D->addAttr(::new (S.Context) + NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident)); ---------------- MForster wrote: > aaron.ballman wrote: > > 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.) > > Shouldn't we also be validating that what we found is an NSString that has > > the correct properties? > > Is your suggestion to string-compare the name of the type? >>Shouldn't we also be validating that what we found is an NSString that has >>the correct properties? > Is your suggestion to string-compare the name of the type? You should be able to compare the `QualType` of the resulting `VarDecl` against `ASTContext::getObjCNSStringType()` (accounting for qualifiers, pointers, etc). ================ 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: > > 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). > > 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). > > How are you suggesting to implement that? Change the argument to to be a > `DeclArgument` or `ExprArgument` instead of an `IdentifierArgument`? >>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). > How are you suggesting to implement that? Change the argument to to be a > DeclArgument or ExprArgument instead of an IdentifierArgument? I think `DeclArgument` is probably the correct approach and you should be able to model it somewhat off the `cleanup` attribute. Otherwise, you can gin up a "fake" attribute argument. Those aren't arguments used by the parser or pretty printer, but are used to form the semantic attribute constructor to provide additional information. 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
