Szelethus added a comment.
Ah, okay I think I got why you chose this direction. Summaries are little more
then a collection of constraints, and at select points in the execution we
check and apply them one-by-one. If we want to preserve this architecture (and
it seems great, why not?), inheritance is indeed the correct design decision.
The costliest code to run according to my limited knowledge about C++
development is indeed the one that is hard to understand and maintain, and this
seems to have a good bit of thought behind it. I'll check your followup patches
to gain some confidence before formally accepting, but the general idea seems
great.
================
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;
+
----------------
Why did we remove the initialization here? Can we make this `constexpr`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:90
+
+ class ValueConstraint {
+ public:
----------------
We should totally have a good bit of documentation here.
================
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:
> 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.
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
