[PATCH] [lld] Teach LLD how to parse complete linker scripts

Rui Ueyama ruiu at google.com
Sun Oct 26 23:42:54 PDT 2014


Seems this patch is good enough to land. We may be able to improve it even more but that can be done in a different patch.

As you mentioned, this patch was too large. It could have been split into small incremental patches, each adding syntactic element one by one. This patch was fortunately not risky, but if it was and we couldn't agree that this should land, you might have wasted your time. So please start with a small patch next time. Thanks!

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:42-68
@@ -40,1 +41,29 @@
     r_paren,
+    star,
+    starequal,
+    plus,
+    plusequal,
+    comma,
+    minus,
+    minusequal,
+    slash,
+    slashequal,
+    number,
+    colon,
+    semicolon,
+    less,
+    lessequal,
+    lessless,
+    lesslessequal,
+    equal,
+    equalequal,
+    greater,
+    greaterequal,
+    greatergreater,
+    greatergreaterequal,
+    question,
+    identifier,
+    libname,
+    kw_align,
+    kw_align_with_input,
+    kw_as_needed,
----------------
shankarke wrote:
> rafaelauler wrote:
> > shankarke wrote:
> > > All of the above need to have a prefix.Also you may need to sort all the enumerated constants like others to be consistent.
> > > 
> > > http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> > I just kept the coding style I found in this file, whose original author is not me. For me, it's no problem to convert this to the official LLVM naming convention.
> > 
> > This enum is already sorted according to Rui's suggestion, by using the ASCII table order.
> > 
> > Correct me if I'm wrong, but I think we can drop the prefix here, according to official LLVM naming rules. If the enum is inside a class, the prefix is not necessary.
> We are following a mixed pattern here, for some we use a kw_ to start the enumeration with, there are _(underscore patterns) and non underscore patterns, I am not sure on what we want to rename but I think it would be nice if we are consistent in naming the enumerations ?
Seems only keyword (e.g. "align" or "as_needed") starts with kw_ prefix. That looks fine and this code follows the coding style. I don't see non-keyword (e.g. = or +) needs prefix.

================
Comment at: include/lld/ReaderWriter/LinkerScript.h:434
@@ +433,3 @@
+public:
+  TernaryConditional(const Expression *conditional, const Expression *trueExpr,
+                     const Expression *falseExpr)
----------------
shankarke wrote:
> explicit again.
This constructor has three parameters so explicit keyword is not needed.

http://reviews.llvm.org/D5852






More information about the llvm-commits mailing list