[PATCH] D152833: A new code layout algorithm for function reordering [1/3]

[Hongtao Yu via Phabricator via llvm-commits]

hoy accepted this revision.
hoy added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:161
+/// together with the corresponding merge 'type' and 'offset'.
+struct MergeGainT {
+  explicit MergeGainT() = default;
----------------
spupyrev wrote:
> hoy wrote:
> > spupyrev wrote:
> > > hoy wrote:
> > > > nit: by convention the `T` in the struct name is not needed. I'm seeing `JumpT` and a few other types getting `T`. Is there a particular reason?
> > > I too don't like this T suffix but I found myself writing
> > > ```
> > >     for (Chain *Chain : SortedChains)
> > > ```
> > > all over the place. It compiles but looks confusing. I strongly prefer `ChainT *Chain` over this instead.
> > > 
> > > Any other suggestions?
> > I usually do 
> > 
> > 
> > ```
> >  for (auto *Chain : SortedChains)
> > ```
> > 
> > or
> > 
> > ```
> >  for (Chain *C : SortedChains)
> > ```
> I've been using the former for a while but Some LLVM folks advised me against over-using `auto` for non-complex types. I've even seen constructs `auto *Chain` refactored to `class Chain *Chain` in some cases, which is worse than introducing `ChainT`, in my opinion.
> I feel like providing full variable names, such as `Chain` in instead of `C`, yields more readable code, no?
Yeah, full name also sounds good to me. In that case, I would name `Chain` to `BlockChain` or something.

It may be not easy to get every case working like that. TBH, it's also annoying to me sometimes to come up with a tradeoff between type names and variable names :). 

I'm not sure why type name suffix like `T` is not preferred in this code base. I don't have a strong opinion. It's up to you to check in as is or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152833