[PATCH] D73888: [yaml2obj][obj2yaml] - Simplify format of the SHT_LLVM_ADDRSIG section.

[George Rimar via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-readobj/ELF/addrsig.test:23
     Type: SHT_LLVM_ADDRSIG
-    Symbols:
-      - Name: foo
-      - Name: bar
+    Symbols: [ foo, bar ]
 Symbols:
----------------
jhenderson wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > Thinking about his again, I wonder if we want the following output format:
> > > > ```
> > > > Symbols:
> > > >   - foo
> > > >   - bar
> > > > ```
> > > > 
> > > > All what is needed is to stop using `YAMLFlowString` and use `StringRef`,
> > > > so the change is trivial.
> > > > That might produce a nicer (?) output when symbol names are long probably,
> > > > though I am not sure we should care too much here and/or what is better.
> > > Is there a test having a special symbol name (a metacharacter in YAML.)? e.g. `[` 
> > > 
> > > Just want to make sure they are properly quoted.
> > A list of `YAMLFlowString` can be wrapped (default wrap column is 70), can't it?
> > 
> > If yes, I think `YAMLFlowString` is fine. I don't have a strong opinion, though. We can also use `StringRef`.
> I personally prefer the `YAMLFlowString` syntax, as long as it can be wrapped as @MaskRay mentions. I think we made a similar change in recent months in that direction for something else, but I don't remember what.
> 
> However, if there's strong reasoning to favour the `StringRef` approach, I don't mind either.
> Is there a test having a special symbol name (a metacharacter in YAML.)? e.g. [

If I correctly understood the idea, we have no tests for symbol like "[aabb", "aa[bb" etc yet.




================
Comment at: llvm/test/tools/llvm-readobj/ELF/addrsig.test:23
     Type: SHT_LLVM_ADDRSIG
-    Symbols:
-      - Name: foo
-      - Name: bar
+    Symbols: [ foo, bar ]
 Symbols:
----------------
grimar wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > MaskRay wrote:
> > > > grimar wrote:
> > > > > Thinking about his again, I wonder if we want the following output format:
> > > > > ```
> > > > > Symbols:
> > > > >   - foo
> > > > >   - bar
> > > > > ```
> > > > > 
> > > > > All what is needed is to stop using `YAMLFlowString` and use `StringRef`,
> > > > > so the change is trivial.
> > > > > That might produce a nicer (?) output when symbol names are long probably,
> > > > > though I am not sure we should care too much here and/or what is better.
> > > > Is there a test having a special symbol name (a metacharacter in YAML.)? e.g. `[` 
> > > > 
> > > > Just want to make sure they are properly quoted.
> > > A list of `YAMLFlowString` can be wrapped (default wrap column is 70), can't it?
> > > 
> > > If yes, I think `YAMLFlowString` is fine. I don't have a strong opinion, though. We can also use `StringRef`.
> > I personally prefer the `YAMLFlowString` syntax, as long as it can be wrapped as @MaskRay mentions. I think we made a similar change in recent months in that direction for something else, but I don't remember what.
> > 
> > However, if there's strong reasoning to favour the `StringRef` approach, I don't mind either.
> > Is there a test having a special symbol name (a metacharacter in YAML.)? e.g. [
> 
> If I correctly understood the idea, we have no tests for symbol like "[aabb", "aa[bb" etc yet.
> 
> 
>A list of YAMLFlowString can be wrapped (default wrap column is 70), can't it?
> If yes, I think YAMLFlowString is fine

Yep. here is the output from obj2yaml I had when tested it:
```
Symbols:         [ foo, bar, foo, bar, foo, bar, foo, bar, foo, bar,
                   foo, bar, foo, bar, foo, bar, foo, bar ]
```


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

https://reviews.llvm.org/D73888