[PATCH] D85808: [remarks] Optimization remarks hotness filtering from profile summary

Thu Aug 13 17:19:03 PDT 2020

tejohnson added inline comments.

================
Comment at: lld/ELF/Config.h:280
+  // If threshold option is not specified, it is disabled by default.
+  llvm::Optional<uint64_t> optRemarksHotnessThreshold = 0;

----------------
weiwang wrote:
> tejohnson wrote:
> > Since this field is being added in patch D85809 as an unsigned, why not add it as llvm::Optional<> there to start with instead of adding and then changing?
> > 
> > Ditto for a number of other places in this patch.
> Thanks for the feedback! It does seem awkward. When doing the splitting, I tried to make the 3 patches look more self-contained from each other. The `Optional` type change seems unrelated with the first patch. 
I would recommend going straight to Optional in the first patch, and just note there that the type is needed for the follow on patch. That will avoid unnecessary churn.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85808