Thanks for the feedback! Definitely, anonymous namespaces should only be suggested in C++. That might complicate the logic and testing. An alternative solution would be to not suggest any fix and provide a more generic message, like "this function has external linkage but the declaration is missing, please add it or make the function have internal linkage if it's only meant to be used in this translation unit".
The downside to that is then we lose functionality that was useful in
all language modes.
> the fix cannot be automatically applied by an engine
Interesting, I wasn't aware there are tools that automatically fix compiler warnings, is that an LLVM tool?
clang-tidy has the ability to apply fix-its as part of its public
interface (see `--fix`), as does clang, though that's part of the CC1
interface (`-Xclang -fixit`).
If that's the case, I suppose removing the fix-it would cause trouble to people relying on it. What concerns me is the compiler implicitly making a style decision for the user when it has two valid options to choose from - I think this fits better in e.g. clang-tidy. The compiler can have one responsibility - detect functions that may accidentally have external linkage, and then the linter would be in charge of automatically applying one style or the other. A middle point could be to keep the fix-it only for C (since "static" is the only choice) and not have it for C++ (just have the generic message as above).
I agree that it could be argued that this is a style choice, but I
guess I'm not really sold on that being what's happening here. It's a
warning diagnostic and we encourage people to provide a fix-it for
warnings that can be unambiguously silenced with little effort. Making
a declaration be `static` is a trivial code transformation that will
correctly silence the diagnostic in all language modes. Wrapping in
anonymous namespaces is considerably more involved (What's the scope
of the namespace? Do you try to merge with adjacent anonymous
namespaces?) and it only works for C++-based language modes. So to me,
Clang's choice isn't about enforcing a style (aside from the style
that the user wants enforced regarding having a prototype before a
definition), it's about silencing the diagnostic in a practical,
Adding a fix-it to suggest anonymous namespaces may also be practical
and maintainable, of course. I mostly suspect we went with `static`
because it was low-hanging fruit that was trivial to implement, but we
could probably dig up the original review if we wanted to see if
anonymous namespaces were discussed and rejected for some reason.
> another solution for your situation might be a clang-tidy check to enforce your company's coding guidelines
Nice idea, this works too! In order to not duplicate code, I was thinking to have a check that enforces static -> anonymous namespace, still leaving the responsibility of "detecting incorrect external linkage" to clang. This might generate some developer friction though, as they first stumble across the clang warning, fix it with "static" and later on in CI get the clang-tidy warning telling them to put it in the anonymous namespace.
Yeah, we run into this sort of "which layer should report the issue"
friction with clang-tidy checks from time to time. In practice, I'm
not aware of the friction being too onerous (we usually try to add
tidy check options to help people out when requested), but your
mileage may vary.