[PATCH] D99646: [clang-tidy] misc-std-stream-objects-outside-main: a new check

[Marco Gartmann via Phabricator via cfe-commits]

mgartmann marked 10 inline comments as done.
mgartmann added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp:25
+          .bind("match"),
+      this);
+}
----------------
riccibruno wrote:
> Will this match `my_namespace::cin`?
Yes, at the moment this would be matched as well.


================
Comment at: clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp:42
+
+bool StdStreamObjectsOutsideMainCheck::isInsideMainFunction(
+    const MatchFinder::MatchResult &Result, const DynTypedNode &Node) {
----------------
njames93 wrote:
> This all seems unnecessary, you can add `unless(forFunction(isMain()))` to your declRefExpr matcher instead.
Thanks for pointing this out to me. `forFunction()` is exactly what I was initially looking for. Unfortunately, I did not find it. 

I now refactored the code to use this matcher instead.

I guess `isMain()` would not match if a MSVCRT entry point (see @riccibruno's answer on line 47) is used, is it? Do you think it makes sense to also match this kind of entry point? How could this be done? I was not able to find anything related in the [[ https://clang.llvm.org/docs/LibASTMatchersReference.html | AST Matcher Reference ]].


================
Comment at: clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp:47
+  if (AsFunctionDecl && AsFunctionDecl->getIdentifier() &&
+      AsFunctionDecl->getName().equals("main")) {
+    return true;
----------------
riccibruno wrote:
> You can use `FunctionDecl::isMain`. Additionally you might want to also use `FunctionDecl::isMSVCRTEntryPoint` for the Windows-specific main functions.
Due to @njames93's suggestion to use a `unless(forFunction(isMain()))` matcher, this function is not needed anymore.

Thank you anyways for pointing this out to me. 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-std-stream-objects-outside-main.cpp:1
+// RUN: %check_clang_tidy -std=c++14-or-later %s misc-std-stream-objects-outside-main %t
+
----------------
njames93 wrote:
> Why won't this work in c++11 mode?
When not specifying C++14 as the standard, the corresponding tests failed.
Some errors in the test output pointed out that e.g.,// auto return without trailing return types// is a C++14 extension.

I created a [[ https://gist.github.com/mgartmann/f8a876ebbed6f7c1b8e7a9c901f36508 | gist (click me) ]] which contains the test ouput of this clang-tidy check. There, all occured errors can be seen.

IMHO, those errors seem to come from the directly or indirectly included header files rather than from this check. Please correct me if I am wrong.

After adding `-std=c++14-or-later`, no errors occured anymore. That is the reason why I added it.

Anyways, I now refactored the test file, creating a `std` namespace and adding my own "mocked" stream objects. I adpoted this approach from existing tests (e.g., `readability-string-compare`).

Thus, no include is needed anymore and `-std=c++14-or-later` can be omitted.

If I overlooked something, I would be glad if you could point this out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99646