[PATCH] D44004: [ELF] - Show data and assignment commands in the map file.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.h:110-111
+
+  // Following attributes are used for printing the map file.
+  // Keeps string representing the command.
+  std::string CommandString;
----------------
It is obvious that this comment describe the following variable, so you don't need to mention that.

  // A string representation of this command. We use this for -Map.


================
Comment at: ELF/LinkerScript.h:113
+  std::string CommandString;
+  // The offset value within the section before execution of assignment.
+  unsigned Offset;
----------------
Please insert a blank line before a comment.


================
Comment at: ELF/LinkerScript.h:113
+  std::string CommandString;
+  // The offset value within the section before execution of assignment.
+  unsigned Offset;
----------------
ruiu wrote:
> Please insert a blank line before a comment.
What do you mean by "before execution of assignment"? Before you assign a command to an output section, it doesn't have any address.


================
Comment at: ELF/MapFile.cpp:154
+      if (auto *Cmd = dyn_cast<ByteCommand>(Base)) {
+        writeHeader(OS, OSec->Addr + Cmd->Offset, Cmd->Size, Cmd->Size);
+        OS << Indent1 << Cmd->CommandString << '\n';
----------------
You are passing `Cmd->Size` as an alignment which seems wrong to me.


================
Comment at: ELF/ScriptParser.cpp:118-119
+  // Concatenates tokens in a [FirstTok, LastTok] into string with Prefix.
+  std::string buildCommandString(StringRef Prefix, size_t FirstTok,
+                                 size_t LastTok);
+
----------------
FirstTok and LastTok are not tokens but positions, so you should avoid "Tok".


================
Comment at: ELF/ScriptParser.cpp:782
+                                             size_t LastTok) {
+  return (Prefix + " ").str() +
+         llvm::join(Tokens.begin() + FirstTok, Tokens.begin() + LastTok, " ");
----------------
This looks weird -- at least I believe you could do `Prefix.str() + " " + ...`.


================
Comment at: ELF/ScriptParser.cpp:796-797
+
+  SymbolAssignment *Ret = make<SymbolAssignment>(Name, E, getCurrentLocation());
+  Ret->CommandString = buildCommandString(Name, OldPos, Pos);
+  return Ret;
----------------
You should change the ctor so that it takes CommandString as the third parameter. Half-initializing an object using the constructor and then set a new value to a member looks weird.


https://reviews.llvm.org/D44004