[PATCH] D113839: [NFC] Use Optional<ProfileCount> to model invalid counts

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/IR/Function.h:261-265
     ProfileCount(uint64_t Count, ProfileCountType PCT)
         : Count(Count), PCT(PCT) {}
-    bool hasValue() const { return PCT != PCT_Invalid; }
     uint64_t getCount() const { return Count; }
     ProfileCountType getType() const { return PCT; }
     bool isSynthetic() const { return PCT == PCT_Synthetic; }
----------------
mtrofin wrote:
> dblaikie wrote:
> > Could this just be a struct at this point, maybe with an "isSynthetic" free function if testing "type == Synthetic" is too verbose?
> > 
> > The members are all both directly accessible, and can be initialized to any value, so the encapsulation doesn't seem to be providing anything?
> The value of encapsulation here is that it clarifies when the members are set: at construction only. Makes it so much easier (to me at least) to read the code by not forcing one to chase all references to a field and see whether it is set or read.
any piece of code could still change the values with: `someCount = ProfileCount(a, b)` - and it doesn't look like these values are used in particularly complicated ways, most have a scope of maybe 5 lines (which is great - and cases where they have long/complicated scopes more at risk of spooky-action-at-a-distance - refactoring to reduce the length of functions/scopes, pull out pieces into separate functions would be good)

This isn't done for most types in C++ because it runs up against C++ design idioms. Some codebases might use top-level const on local variables more often to enforce this & it's certainly where I'd go if I were suggesting addressing this issue - but it does add a lot of extra text/verbosity, and also isn't really done in the LLVM codebase (It's done in a smattering of places, but I think the inconsistency is also unhelpful - there's loads of values that could be changed so reducing scopes in general probably has more payoff across all those values)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113839/new/

https://reviews.llvm.org/D113839