[PATCH] D68062: Propeller lld framework for basicblock sections

Fri Sep 27 17:27:24 PDT 2019

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

================
Comment at: lld/ELF/Propeller.cpp:27
+#include "Config.h"
+#include "PropellerBBReordering.h"
+#include "PropellerELFCfg.h"
----------------
MaskRay wrote:
> PropellerBBReordering.h is added in D68073, not in this patch. Please make each patch compilable.
There are 3 patches in lld, and our plan is to submit all in one shot, so we marked patch relationships (parent/child) for all these 3 . (if phabricate does not support this operation, we'll submit it locally) Does this seem reasonable to you?

================
Comment at: lld/ELF/Propeller.h:1
+#ifndef LLD_ELF_PROPELLER_H
+#define LLD_ELF_PROPELLER_H
----------------
MaskRay wrote:
> MaskRay wrote:
> > Add license notice.
> > Use variableName instead of VariableName to conform to the local naming convention.
> > Use variableName instead of VariableName to conform to the local naming convention.
> 
> This is not done.
A little bit confused. https://llvm.org/docs/CodingStandards.html states that noun name should be camel cased, with first letter in upper case. However I also see LLD files have used "smallUpperCaseNames" as locals...

I'll conform to lld.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062