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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Happy to approve the incremental improvement, even if it isn't as far as I'd go - up to you on the struct/class discussion.



================
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:
> 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.
(I think we're on the same page - discussing whether this could/should be a naked struct with two members, rather than having accessor member functions and private (albeit non-const) member variables)

Perhaps a reasonable summary of my perspective is: Since the values can be changed (by writing `someCount = ProfileCount(a, b)`) the read-only accessors don't provide the safety that a reader can assume the members are not modified - and we don't have that safety around most values/variables in C++ code, so I'm not sure why it's valuable to have it for this one type when all the other locals are modifiable (& this one is too, albeit slightly awkwardly by using assignment to the whole value rather than per-member).

The gain, to my mind, is a simpler API - like a function returning an int, we wouldn't usually wrap that in some other type that only provides read-access to it. This isn't much different.

It doesn't seem to me it provides the reader benefit you mentioned/motivated this design - that the reader doesn't have to worry about other mutations (because there can be mutations, by using the assignment) - and the scopes this exists in are quite small, so that reader benefit seems small and it's only one type/variable among many other mutable ones so I'm not sure it provides much value compared to reasoning about the rest of the variables/code in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113839