aaron.ballman accepted this revision. aaron.ballman added a comment. In D149650#4319461 <https://reviews.llvm.org/D149650#4319461>, @sammccall wrote:
> In D149650#4316138 <https://reviews.llvm.org/D149650#4316138>, @aaron.ballman > wrote: > >> In D149650#4315211 <https://reviews.llvm.org/D149650#4315211>, @sammccall >> wrote: >> >>> In D149650#4312389 <https://reviews.llvm.org/D149650#4312389>, >>> @aaron.ballman wrote: >>> >>>> I guess I'm not seeing much motivation for this change. We already have >>>> `clang::getNullabilitySpelling()` and `const StreamingDiagnostic >>>> &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind >>>> nullability)` and now we're adding a third way to get this information. If >>>> this is just for debug/testing purposes, can we use existing debug >>>> formatters to convert the enumeration value into the enumerator name >>>> instead? >>> >>> We're using NullabilityKind in our dataflow-based null-safety clang-tidy >>> check <https://github.com/google/crubit/tree/main/nullability> that we hope >>> to upstream when it works. >> >> Ah, good to know! I think it would be reasonable to add this functionality >> once there's some agreement on adding the clang-tidy check. >> >>> (The idea is to use `_Nullable` and `_Nonnull` annotations on API >>> boundaries to gradually type pointers, and to provide a verification >>> clang-tidy check and libraries to infer annotations for existing code. The >>> actual clang-tidy check wrapper isn't in that repo yet, but it's trivial). >>> >>> It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, >>> ElementsAre(Nullable, Unspecified)) >>> <https://github.com/google/crubit/blob/main/nullability/type_nullability_test.cc> >>> printed something useful when it fails, if we could write OS << NK and get >>> Unspecified rather than OS << getSpelling(NK) and get >>> _Unspecified_Nullability, etc. >>> This doesn't concern clang core (ha, there are no unit tests...) but >>> there's no reasonable way to do this downstream without using a different >>> type. >> >> Hmmm, but does this need to be added to `Specifiers.h` instead of being kept >> packaged up with clang-tidy? For things like AST matchers, we usually ask >> that the matcher remain local to the clang-tidy check unless the same >> matcher is needed in multiple checks. This feels somewhat similar, despite >> it not being related to AST matching -- if the only need for this >> functionality exists out of the clang tree, can we keep it out of the clang >> tree until we find more needs? > > This can't be added downstream, the type needs to provide these. > ADL extension points like operators need to be in namespace `clang`[1] so > that ADL will find them. If non-owners of the type define these, this ends up > in violating ODR when two such non-owners are linked together. Hacking around > this for the `operator<<` symbol itself (e.g. putting it inline in anonymous > namespaces) results in ODR violations in templates that call them (in this > case, gtest). Even defining them centrally in a separate header is risky, > including/not including the header produces similar violations. Oh shoot, you're right. I hadn't thought about the fact that this is an ADL extension point specifically. > There's never going to be a hard need for this - having an ergonomic way to > print things that composes with the rest of our infra is useful but it's > always possible to live without it. > If we don't want such clang-as-a-library features upstream that's OK. We can > define our own `NullabilityKind` and marshal between them - I need to be able > to answer "why are we hacking around clang rather than improving it" in code > review :-) I think you've convinced me this is reasonable to keep upstream despite not really being used in tree (yet). If it turns out that the clang-tidy check doesn't get upstreamed, we can remove this easily enough in the future if needed. I don't think we want to expose this sort of thing as a matter of course, but when the alternatives are significantly more dangerous, it is reasonable. If we find we're adding more clang-as a-library features upstream like this, we might want to revisit whether there's a better design approach for these needs. LG! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149650/new/ https://reviews.llvm.org/D149650 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
