[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Hmm, okay, so we convert `-1` from the config file to `UINT_MAX` in the code, I like it!

I wrote a couple nits but they really are just that. In general, for each different error message, a different test case would be great.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:805
+                  "Config",
+                  "Specifies the name of the configuration file.",
+                  "",
----------------
Okay, so how do I know what the format of that file is? How about we create another option (in a different revision) that dumps an example configuration with comments?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:27
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
-#include <climits>
-#include <initializer_list>
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/StringMap.h"
----------------
Do we need this include?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:89-92
+  static const unsigned InvalidArgIndex{std::numeric_limits<unsigned>::max()};
   /// Denotes the return vale.
-  static const unsigned ReturnValueIndex = UINT_MAX - 1;
+  static const unsigned ReturnValueIndex{std::numeric_limits<unsigned>::max() -
+                                         1};
----------------
Leave this as is for now, but maaybe in the future we should just use an enum. What do you think?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:304-306
+      Mgr.reportInvalidCheckerOptionValue(
+          this, Option,
+          "an argument number for propagation rules greater or equal to -1");
----------------
Could we have a test for this too? :)


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:1
+//== Yaml.h ----------------------------------- -*- C++ -*--=//
+//
----------------
Have with with the same length as the rest :)


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:9
+//
+// This file contains a simple interface allow to open and parse yaml files.
+//
----------------
Is this actually the reason why we have this file? We already have a YAML parser in LLVM, what's does this file do that the "default" parser doesn't?


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:14
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/Support/YAMLTraits.h"
----------------
Hmmm, do we need this include?


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:16
+#include "llvm/Support/YAMLTraits.h"
+
+/// Read the given file from the filesystem and parse it as a yaml file. The
----------------
```lang=c++
namespace clang {
namespace ento {
```


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:42-43
+  if (std::error_code ec = Input.error()) {
+    Mgr.reportInvalidCheckerOptionValue(Chk, Option,
+                                        "a valid yaml file: " + ec.message());
+    return {};
----------------
And for this too?


================
Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:48
+  return Config;
+}
----------------
```lang=c++
} // end of namespace clang
} // end of namespace ento
```


================
Comment at: test/Analysis/taint-generic.c:24
+// CHECK-INVALID-FILE-SAME:        'alpha.security.taint.TaintPropagation:Config',
+// CHECK-INVALID-FILE-SAME:        that expects a valid filename
 
----------------
Could you please add the rest of the error message?


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

https://reviews.llvm.org/D59555