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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
