baloghadamsoftware added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:102
+ /// Given a range, should the argument stay inside or outside this range?
+ enum RangeKind { OutOfRange, WithinRange };
----------------
I would move this into the class to encapsulate the values instead of
contaminating namespace `ento`.
================
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:
> > 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.
================
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:
> 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.
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
