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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin 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:
> > 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)
> But profileCount.Count = 5 means nothing - i.e. the point of the type is to encapsulate something coming from metadata. so what would be the usecase for freely setting the flags? Meaning, what is lost with the current design?
> 
> I get the not const-ing the fields for scenarios like re-assigning a local var. They aren't const anymore.
> But profileCount.Count = 5 means nothing - i.e. the point of the type is to encapsulate something coming from metadata. so what would be the usecase for freely setting the flags? Meaning, what is lost with the current design?
> 
> I get the not const-ing the fields for scenarios like re-assigning a local var. They aren't const anymore.

Oh, maybe there's a misunderstanding: my argument was about not going to a naked struct, not about const fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113839