Szelethus 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. ---------------- 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 :) 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
