aaron.ballman added inline comments.

================
Comment at: clang-tidy/bugprone/BugproneTidyModule.cpp:84
+    CheckFactories.registerCheck<IoFunctionsMisusedCheck>(
+        "bugprone-io-functions-misused");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
----------------
This name reads a bit awkwardly because usually "misused" would come before "io 
functions". However, given that we've already said it's prone to bugs, I think 
"misuse" is somewhat implied. Perhaps `bugprone-io-functions` or 
`bugprone-misused-io-functions'?

However, I think this is actually checking an existing CERT rule and perhaps 
should be added to the CERT module as `cert-fio34-c` and perhaps that's the 
only module it should live in?


================
Comment at: clang-tidy/bugprone/IoFunctionsMisusedCheck.cpp:25-31
+              has(callExpr(callee(
+                  functionDecl(hasAnyName("::getwchar", "::getwc", "::fgetwc"))
+                      .bind("FuncDecl")))),
+              has(callExpr(callee(
+                  functionDecl(hasAnyName("::std::getwchar", "::std::getwc",
+                                          "::std::fgetwc"))
+                      .bind("FuncDecl")))),
----------------
You can combine these `hasAnyName()` calls into one given that they are 
otherwise identical matchers.


================
Comment at: clang-tidy/bugprone/IoFunctionsMisusedCheck.cpp:26
+              has(callExpr(callee(
+                  functionDecl(hasAnyName("::getwchar", "::getwc", "::fgetwc"))
+                      .bind("FuncDecl")))),
----------------
Why only the wide char versions and not the narrow char versions as well? For 
instance, those are problematic on systems where `sizeof(int) == sizeof(char)`. 
See 
https://wiki.sei.cmu.edu/confluence/display/c/FIO34-C.+Distinguish+between+characters+read+from+a+file+and+EOF+or+WEOF
 for more details.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42682



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D42682: [clang... Aaron Ballman via Phabricator via cfe-commits

Reply via email to