[cfe-dev] [clang][RFC] -Wmissing-prototypes: make suggested fix more generic?

Sat Dec 11 11:32:54 PST 2021

> as does clang, though that's part of the CC1 interface (`-Xclang -fixit`).
Amazing, didn't know about that one, thanks!

> it's about silencing the diagnostic in a practical, maintainable manner.
I agree, it's good to have a simple solution that works for both languages.

> Adding a fix-it to suggest anonymous namespaces may also be practical
For the case of C++ I think having a fix-it for anonymous namespaces would
be
just as problematic -- some other coding guidelines (e.g. the LLVM ones)
will prefer "static" instead. As you say there's also quite more complexity
to handle,
which may risk raising the FP rate to levels not acceptable for compiler
warnings.
In that case I'd prefer to not have any fix-it and just present the user
with both
alternatives, but I see how we wouldn't want to lose existing functionality.

> we could probably dig up the original review
I had a look at the review <https://reviews.llvm.org/D59402> and couldn't
find any mention of anonymous namespaces,
probably as you say "static" was just the lowest hanging fruit.

I think you've convinced me to go the clang-tidy route so I'll take a look
at that instead.
Thanks a lot for the feedback, really appreciated!

/Carlos

On Sat, Dec 11, 2021 at 7:18 PM Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Sat, Dec 11, 2021 at 11:51 AM Carlos Galvez <carlosgalvezp at gmail.com>
> wrote:
> >
> > 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,
> maintainable manner.
>
> 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.
>
> ~Aaron
>
> >
> > /Carlos
> >
> > On Fri, Dec 10, 2021 at 7:04 PM Aaron Ballman <aaron at aaronballman.com>
> wrote:
> >>
> >> On Fri, Dec 10, 2021 at 8:25 AM Carlos Galvez via cfe-dev
> >> <cfe-dev at lists.llvm.org> wrote:
> >> >
> >> > Hi,
> >> >
> >> > We want to enable the warning -Wmissing-prototypes to enforce
> internal linkage as much as possible. It works great, with one caveat - it
> suggests to use "static". Internal linkage can also be achieved with
> anonymous namespaces and Clang will not issue a warning.
> >> >
> >> > Now, the question as to what to use (static vs anonymous namespaces)
> is mostly a style issue (a "Microscopic Detail" in the LLVM Coding
> Guidelines). At our company we have decided to prefer anonymous namespaces
> over static, so we wouldn't want Clang to start suggesting people
> (especially junior devs that trust the tools blindly) to use static.
> >> >
> >> > Therefore I wanted to ask here - would it be possible to make the
> suggestion more generic, maybe displaying both alternatives? Best would be
> making this user-configurable but I'm guessing that's not a trivial thing
> to do.
> >> >
> >> > I'm happy to implement the changes given some pointers, but I want to
> check here first if you'd agree with it.
> >>
> >> There are a couple of things to keep in mind.
> >>
> >> The current fix-it is valid in both C and C++, but anonymous
> >> namespaces are C++ only and so that suggestion would be inappropriate
> >> to make in C.
> >> Multiple fixits means the fix cannot be automatically applied by an
> >> engine, whereas the current behavior can.
> >> Clang does not have a way to configure diagnostic options aside from
> >> the command line. I don't think it would be worth the effort to
> >> support something like `-Wmissing-prototypes=prefer-static` or
> >> something like that to parameterize this diagnostic.
> >>
> >> I don't think any of these are necessarily a show-stopper for the
> >> proposed functionality, but they're worth keeping in mind. However,
> >> for me personally, I think the current functionality is sufficient (it
> >> works in all language modes, it's automatable as a fix-it, etc) and
> >> that a perhaps another solution for your situation might be a
> >> clang-tidy check to enforce your company's coding guidelines (which
> >> should hopefully integrate nicely into a CI pipeline for you, can also
> >> automatically apply fix-its, can be tailored to your specific needs,
> >> etc).
> >>
> >> ~Aaron
> >>
> >> >
> >> > Thanks!
> >> > /Carlos
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > cfe-dev mailing list
> >> > cfe-dev at lists.llvm.org
> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211211/2f663f42/attachment-0001.html>