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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
