[PATCH] D73045: [obj2yaml] - Better indentations in the ELF output.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Okay, I'm happy with the concept, and I don't think it's too complex, but I do think my inline comments about the formatting for lists (members, and also other keys in the list owner) need resolving for this to be properly worthwhile.



================
Comment at: llvm/include/llvm/Support/YAMLTraits.h:1133
 
+  void setOptimizeIndentations(bool Optimize);
+
----------------
Some doxygen-style comments on this and the other new functions and classes are needed.


================
Comment at: llvm/include/llvm/Support/YAMLTraits.h:1139
+
+  void setMaxPadding(int Val) { MaxPadding = Val; }
+
----------------
Should this be a size_t? Negative padding doesn't make much sense...


================
Comment at: llvm/include/llvm/Support/YAMLTraits.h:1206
   SmallVector<InState, 8> StateStack;
+  SmallVector<int, 8> MaxPaddingsStack;
   int Column = 0;
----------------
MaxPaddingsStack -> MaxPaddingStack


================
Comment at: llvm/include/llvm/Support/YAMLTraits.h:1217
   StringRef PaddingBeforeContainer;
+  std::unique_ptr<PreMappingOutput> PreMappingOut;
+};
----------------
This seems like a weird way of structuring the code. A PreMappingOutput is a subclass of Output, but that means it also contains a unique pointer to itself? That seems like a recipe for destruction order issues.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:833
+
+  static std::string Spaces(16, ' ');
+  int PaddingLen;
----------------
Why not just `StringRef Spaces = "                ";`?


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:836
+  if (MaxPadding == -1)
+    PaddingLen = std::max<int>(Spaces.size() - key.size(), 1);
   else
----------------
My immediate thought here was "what if the key is longer than the spaces". I imagine it's handled, but it's not obvious.


================
Comment at: llvm/lib/Support/YAMLTraits.cpp:838
   else
-    Padding = " ";
+    PaddingLen = std::min<int>(std::max<int>(MaxPadding - key.size(), 1), Spaces.size());
+  Padding = StringRef(Spaces).take_front(PaddingLen);
----------------
clang-format?


================
Comment at: llvm/test/tools/obj2yaml/elf-output-indentation.yaml:19
+# CHECK-NEXT:    Type:         SHT_PROGBITS
+# CHECK-NEXT:    AddressAlign: 0x0000000000000001
+# CHECK-NEXT:  - Name:        .rela.text
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > Here the additional indentation appears because of `AddressAlign`, which is
> > > the largest key among others. The same situation is shown below.
> > > I did not investigate how much hard to to improve such places too yet.
> > I definitely find it weird that .text is not indented to the same extend as .data.
> Yes, though it just follows the simple rule: "find the largest key size and align others".
> I am not sure what can be a good generic heuristic without knowing the context.
Is it not possible to iterate over all list elements to find their individual required padding sizes, and then set them all to the max of the list members?


================
Comment at: llvm/test/tools/obj2yaml/elf-output-indentation.yaml:20-24
+# CHECK-NEXT:  - Name:        .rela.text
+# CHECK-NEXT:    Type:        SHT_RELA
+# CHECK-NEXT:    Link:        .symtab
+# CHECK-NEXT:    EntSize:     0x0000000000000018
+# CHECK-NEXT:    Info:        .text
----------------
grimar wrote:
> jhenderson wrote:
> > I'm guessing this block is indented because of Relocations? This doesn't seem great, given that the Reloctions line doesn't have a direct value itself.
> Yeah, but the problem here is that `preflightKey` which is used to scan keys:
> 
> ```
>  bool preflightKey(const char *Key, bool Required, bool SameAsDefault, bool &,
>                     void *&) override {
>     if (Required || !SameAsDefault)
>       MaxKeyLen = std::max(MaxKeyLen, strlen(Key));
>     return false;
>   }
> ```
> 
> does not have any information about values (how to know that `Relocations` is an empty list?)
> Accessing to the values never happens,
> because of `return false`, which prevents any more logic from execution.
> I.e. The code only scans over keys on the current level and does not allow going deeper.
> If it did than much more logic would be needed to be implemented for this "pre-mapping" pass.
> 
> The solutions I see are: either adopting the code in `yaml::Output` to teach it about possibility of "pre-mapping"
> (the idea of this patch was to avoid touching `Output` code, I've only fixed a minor misbehaviors in `processKeyWithDefault`)
> and/or implementing a smarter (and larger) `PreMappingOutput` class.
> 
> 
I'm not really an expert in how all this code fits together, so I don't know what's possible. However, I do think that indenting a value like this looks bad, and will regularly leave us with extra spurious spaces again. Is it possible to tell what kind of mapping keys are (e.g. lists versus strings/numbers) at all? I do think some extra smarts to solve this would be wise if needed.


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

https://reviews.llvm.org/D73045