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

================
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.
----------------
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.


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