[PATCH] D149650: Give NullabilityKind a printing operator<<

[Aaron Ballman via Phabricator via cfe-commits]

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