[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D77461#1977703 <https://reviews.llvm.org/D77461#1977703>, @aaron.ballman wrote:

> In D77461#1977687 <https://reviews.llvm.org/D77461#1977687>, @vingeldal wrote:
>
> > In D77461#1977639 <https://reviews.llvm.org/D77461#1977639>, @aaron.ballman wrote:
> >
> > > In D77461#1977503 <https://reviews.llvm.org/D77461#1977503>, @vingeldal wrote:
> > >
> > > > In D77461#1963166 <https://reviews.llvm.org/D77461#1963166>, @lebedev.ri wrote:
> > > >
> > > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
> > > > >  Diagnosing function-local static non-const variable is a plain false-positive diagnostic.
> > > >
> > > >
> > > > I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?
> > >
> > >
> > > I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.
> > >
> > > > My reason for hesitating to  call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
> > > >  "Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."
> > >
> > > I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).
> >
> >
> > A caller of this function will clearly be affected by other callers if there are any, so there clearly is a dependency added between callers of this function. At the same time the function doesn't provide any hints toward the fact that it is statefull or any way for the caller achieve observability of this state, hence the state causing the dependency is hidden and the dependency is hidden.
> >
> > I must agree that the enforcement section goes against my interpretation but I would prefer to try to simply change the enforcement section.
> >
> > This function with a static variable can almost always be replaced with a solution where the free function is instead made a member function of a class keeping the state as a non-static, private member variable -which would more clearly convey that the function may have state and would provide stronger encapsulation.
> >  The only exception would be if we are talking about a C-interface, in which case one could still use a class, as I just described, instead of a static variable; you would just have to call the member function via a free function.
> >  Can anyone explain to me why one would ever *have* to rely on functions with static variables?
>
>
> To avoid the static initialization order fiasco, statistics or timing counters, etc -- look through the LLVM code base for function local statics, they're not uncommon.
>
> > Because before I can see a legitimate use case for this potential false positive I think it would be a bad idea to consider it a false positive since there are obvious issues with hidden dependencies in the discussed example code.
>
> If we don't hear back from the C++ Core Guidelines authors quickly on their thoughts or adjustments, there are at least two reviewers who consider it a false positive based on the current rule text, which is sufficient post-review feedback to warrant a change IMO.


You can also avoid static initialization order fiasco by not using static as much, but fine, I'll just do it so I don't have to think about this discussion again.
Before I do that I'm just adding to the record that if there was an implementation for rule F.8 this example would definitely been reported by that check instead so it's clearly something to avoid according to the C++ Core Guidelines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77461/new/

https://reviews.llvm.org/D77461