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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/include/llvm/IR/Function.h:257-258
   private:
-    uint64_t Count;
-    ProfileCountType PCT;
-    static ProfileCount Invalid;
+    const uint64_t Count;
+    const ProfileCountType PCT;
 
----------------
dblaikie wrote:
> LLVM code doesn't generally use top-level const. Especially for members this makes things difficult - for instance you can't copy-assign objects with const members (so `ProfileCount a, b; a = b;` would be invalid) which usually isn't intended & can make things harder to work with.
Ack, I can do away with the const here, absence of setters achieves the same thing.


================
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; }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113839