sammccall added inline comments.
================ Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:240 +// Returns the `Value` associated with `E` (which may be either a prvalue or +// glvalue). Creates a `Value` or `StorageLocation` as needed if `E` does not ---------------- mboehme wrote: > sammccall wrote: > > this seems to belong on Environment rather than here! > I've consciously kept this function local to this file for now because it has > relatively complex semantics and I haven't seen any other places where these > are required so far. I've added a comment noting that this should be moved if > it turns out to be needed elsewhere. > > I think that, as much as possible, the methods on `Environment` should be > specific about the value category they operate on, because most of the time, > it's known what the value category is, and I'd like to encourage users to use > the most specific operation possible (because using more general operations > breeds bugs). > I think that, as much as possible, the methods on Environment should be > specific about the value category they operate on I agree, but don't think that's specific to Environment. I think my favorite factoring of this would be to inline the if/else and move the branches to methods on environment ``` Value *Val = E->isGLValue() ? Env.getOrCreateLValue(*E) : Env.getOrCreateValue(*E); ``` but this is test code, it doesn't matter much, up to you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150657/new/ https://reviews.llvm.org/D150657 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
