[PATCH] [lld] [LinkerScript] Implement linker script expression evaluation

[Rui Ueyama]

On Sun, Mar 8, 2015 at 7:57 PM, Shankar Kalpathi Easwaran <
shankarke at gmail.com> wrote:

> ================
> Comment at: include/lld/ReaderWriter/ELFLinkingContext.h:304-306
> @@ -303,1 +303,5 @@
>
> +  const std::vector<std::unique_ptr<script::Parser>> &scripts() const {
> +    return _scripts;
> +  }
> +
> ----------------
> do you want to add a boolean hasLinkerScript that checks the size and
> returns ?
>
> ================
> Comment at: include/lld/ReaderWriter/LinkerScript.h:366
> @@ +365,3 @@
> +  // here is an overkill.
> +  typedef llvm::StringMap<int64_t, llvm::BumpPtrAllocator> SymbolTableTy;
> +
> ----------------
> you could replace this with a llvm::DenseMap<StringRef, int64_t> and use a
> common StringRefMappingInfo.
>

Just use std::map<std::string, T> if this is not in a performance critical
path. Most "optimization" doesn't affect anything to the actual linker
performance. 9 out of 10 cases simplicity is more important than marginal
performance gain.


> ================
> Comment at: include/lld/ReaderWriter/LinkerScript.h:794-795
> @@ -767,2 +793,4 @@
>    void dump(raw_ostream &os) const override;
> +  const_iterator begin() const { return _sectionsCommands.begin(); }
> +  const_iterator end() const { return _sectionsCommands.end(); }
>
> ----------------
> Use lld::range,once you replace it with range, you can remove the typedef
> for const_iterator.
>
> ================
> Comment at: lib/ReaderWriter/LinkerScript.cpp:581
> @@ +580,3 @@
> +  switch (_op) {
> +  case Unary::Minus:
> +    return 0 - childRes;
> ----------------
> Do you want to handle Unary::Plus ??
>
> ================
> Comment at: lib/ReaderWriter/LinkerScript.cpp:584
> @@ +583,3 @@
> +  case Unary::Not:
> +    return ~childRes;
> +  }
> ----------------
> I thought linker script uses only a boolean value of a Unary Not, no ??
>
> ================
> Comment at: lib/ReaderWriter/LinkerScript.cpp:703
> @@ +702,3 @@
> +    return trueExprRes;
> +  else
> +    return falseExprRes;
> ----------------
> no need of else.
>
> ================
> Comment at: unittests/DriverTests/GnuLdDriverTest.cpp:262
> @@ +261,3 @@
> +  EXPECT_EQ(script::SymbolAssignment::Simple, sa1->assignmentKind());
> +  EXPECT_EQ(script::SymbolAssignment::Normal,
> sa1->assignmentVisibility());
> +
> ----------------
> Can we change SymbolAssignment::Normal to SymbolAssignment::Default ?
>
> Looks like you are using the same SymboAssignment to name two different
> things, assignmentKind and assignmentVisibilty. I would recommend
> separating them.
>
> http://reviews.llvm.org/D8156
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150308/85b31455/attachment.html>