martong marked 12 inline comments as done.
martong added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:88
   typedef uint32_t ArgNo;
-  static const ArgNo Ret = std::numeric_limits<ArgNo>::max();
+  static const ArgNo Ret;
+
----------------
Szelethus wrote:
> Why did we remove the initialization here? Can we make this `constexpr`?
I am receiving an undefined reference linker error (I use `gold`) if the 
initialization is happening in-class.  
Even if `constexpr` is used.

```
/usr/bin/ld.gold: error: 
tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/StdLibraryFunctionsChecker.cpp.o:
 requires dynamic R_X86_64_PC32 reloc against 
'_ZN12_GLOBAL__N_126StdLibraryFunctionsChecker3RetE' which may overflow at 
runtime; recompile with -fPIC
tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/StdLibraryFunctionsChecker.cpp.o:StdLibraryFunctionsChecker.cpp:function
 (anonymous 
namespace)::StdLibraryFunctionsChecker::initFunctionSummaries(clang::ento::CheckerContext&)
 const: error: undefined reference to '(anonymous 
namespace)::StdLibraryFunctionsChecker::Ret'
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:90
+
+  class ValueConstraint {
+  public:
----------------
Szelethus wrote:
> We should totally have a good bit of documentation here.
Ok, I added some comments to the class and to `apply` as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:151
+
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
+  /// The complete list of constraints that defines a single branch.
----------------
baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > martong wrote:
> > > > Szelethus wrote:
> > > > > martong wrote:
> > > > > > martong wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > gamesh411 wrote:
> > > > > > > > > martong wrote:
> > > > > > > > > > Note here, we need a copyable, polymorphic and default 
> > > > > > > > > > initializable type (vector needs that). A raw pointer were 
> > > > > > > > > > good, however, we cannot default initialize that. 
> > > > > > > > > > unique_ptr makes the Summary class non-copyable, therefore 
> > > > > > > > > > not an option.
> > > > > > > > > > Releasing the copyablitly requirement would render the 
> > > > > > > > > > initialization of the Summary map infeasible.
> > > > > > > > > > Perhaps we could come up with a [[ 
> > > > > > > > > > https://www.youtube.com/watch?v=bIhUE5uUFOA | type erasure 
> > > > > > > > > > technique without inheritance ]] once we consider the 
> > > > > > > > > > shared_ptr as restriction, but for now that seems to be 
> > > > > > > > > > overkill.
> > > > > > > > > std::variant (with std::monostate for the default 
> > > > > > > > > constructibility) would also be an option  (if c++17 were 
> > > > > > > > > supported). But this is not really an issue, i agree with 
> > > > > > > > > that.
> > > > > > > > Ugh, we've historically been very hostile towards virtual 
> > > > > > > > functions. We don't mind them that much when they don't have to 
> > > > > > > > run a lot, like during bug report construction, but as a core 
> > > > > > > > part of the analysis, I'm not sure what the current stance is 
> > > > > > > > on it.
> > > > > > > > 
> > > > > > > > I'm not inherently (haha) against it, and I'm fine with leaving 
> > > > > > > > this as-is for the time being, though I'd prefer if you placed 
> > > > > > > > a `TODO` to revisit this issue.
> > > > > > > > std::variant (with std::monostate for the default 
> > > > > > > > constructibility) would also be an option (if c++17 were 
> > > > > > > > supported). But this is not really an issue, i agree with that.
> > > > > > > 
> > > > > > > Variant would be useful if we knew the set of classes prior and 
> > > > > > > we wanted to add operations gradually. Class hierarchies (or 
> > > > > > > run-time concepts [Sean Parent]) are very useful if we know the 
> > > > > > > set of operations prior and we want to add classes gradually, and 
> > > > > > > we have this case here.
> > > > > > > Ugh, we've historically been very hostile towards virtual 
> > > > > > > functions. We don't mind them that much when they don't have to 
> > > > > > > run a lot, like during bug report construction, but as a core 
> > > > > > > part of the analysis, I'm not sure what the current stance is on 
> > > > > > > it.
> > > > > > 
> > > > > > I did not find any evidence for this statement. Consider as a 
> > > > > > counter example the ExternalASTSource interface in Clang, which is 
> > > > > > filled with virtual functions and is part of the C/C++ lookup 
> > > > > > mechanism, which is quite on the hot path of C/C++ parsing I think. 
> > > > > > Did not find any prohibition in LLVM coding guidelines neither. I 
> > > > > > do believe virtual functions have their use cases exactly where 
> > > > > > (runtime) polimorphism is needed, such as in this patch.
> > > > > > 
> > > > > I consider myself proven wrong here then.
> > > > Thanks for the review and for considering other alternatives! And 
> > > > please accept my apologies, maybe I was pushing too hard on inheritance.
> > > We should definitely decorate this with a `TODO: Can we change this to 
> > > not use a shared_ptr?`. Worst case scenario, there it will stay for 
> > > eternity :)
> > `FIXME` is the official, not `TODO`, afaik.
> I think that inheritance is the right approach here. However, if it is 
> unacceptable for performance reasons it could be replaced by a template-based 
> solution.
Actually, this is not necessary something that we need to fix or do. So, 
instead of the `TODO` I have added a comment that explains why do we use the 
`shared_ptr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74973



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to