aaron.ballman added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77461/new/ https://reviews.llvm.org/D77461 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
