[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

Tue Dec 6 07:56:51 PST 2022

Szelethus added a comment.

Sorry abour my previous reply, I messed up the thread I was replying to. I better see what is going on.

Do you have a better handle on @martong's previous comment (D135247#3867603 <https://reviews.llvm.org/D135247#3867603>)? Do we know why this strange behaviour occurs with StreamChecker? I assumed that the stream related modeling is done in parallel to that. Is it about the return value?

In D135247#3865357 <https://reviews.llvm.org/D135247#3865357>, @balazske wrote:

> Probably if the `StdLibraryFunctionsChecker` object can be get from `StreamChecker` (a new function is needed) it is possible to check the option and use `CheckerManager::reportInvalidCheckerOptionValue` (for the `StdLibraryFunctionsChecker`). But not sure if it does what we want here.

You can only retrieve a checker object if you can access the class definition of it (as you are aware), so unless you move `StdLibraryFunctionsChecker` to a header file, which doesn't seem like the correct option now (kinda goes against the independent module architecture  (despite us talking about intertwining these "independent" modules)).

For me, the right course seems to be to detach this option from `StdLibraryFunctionsChecker`, and make it an analyzer level option. If this option so severely impacts both checkers, it feels more like an analyzer level option anyways. That will enable you to query the option from `shouldRegisterStreamChecker`. Whether you'd prefer to hard error or only warn about the failure to enable a checker is something I also struggle with, but you could start out with the hard error first.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247