erichkeane added a comment. In D141324#4041159 <https://reviews.llvm.org/D141324#4041159>, @arphaman wrote:
> In D141324#4039629 <https://reviews.llvm.org/D141324#4039629>, @erichkeane > wrote: > >> I'm disturbed that the string-literal diagnostic you changed never shows up >> in the tests. I suspect this attribute needs significantly better test >> coverage. > > Please elaborate. All the diagnostics have appropriate coverage already. I'd mistakenly thought that you'd changed the message to add 'name', not moved it. That said, test coverage of the new feature should be improved. I don't see anything about the contents of the USR, nor anything with the USR field being dependent. ================ Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:58 "for optional message in 'availability' attribute|" - "for %select{language|source container}1 name in " + "for %select{language name|source container name|USR}1 in " "'external_source_symbol' attribute}0">; ---------------- arphaman wrote: > erichkeane wrote: > > I don't think adding 'name' to these makes them particularly more readable? > > At least from what I can see reading it here. > I did not add 'name' here, I moved it from the right hand side as I added a > new clause that didn't use name. Ah! Thanks! I missed that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141324/new/ https://reviews.llvm.org/D141324 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
