MForster marked 3 inline comments as done. MForster added a comment. Two clarifying questions...
================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350 + + D->addAttr(::new (S.Context) + NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident)); ---------------- 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? ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355 + S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl) + << identLoc->Ident; + return; ---------------- 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`? 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
