Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 36219 Build 36218: arc lint + arc unit
Event Timeline
Have you rebased recently? I feel like the content in yaml2macho.cpp may have moved to the ObjectYAML library recently.
llvm/lib/Object/MachOObjectFile.cpp | ||
---|---|---|
1948 | A 32-bit Offset but 64-bit Size seems off to me. What are the limitations on the two fields in Mach-O's file format? | |
llvm/test/ObjectYAML/MachO/section_data.yaml | ||
1 | Probably worth a header comment summarizing the purpose of the test, e.g. "Show that yaml2obj supports custom section data for Mach-O YAML inputs." | |
3 | Nit: add a blank line before the YAML. | |
40 | Move the check before the yaml. Flow should be: ## Comment # RUN: # CHECK: Yaml ## Comment # RUN: # CHECK: Yaml | |
42 | The size of content -> The content size equals to -> equals | |
101 | size of content -> content size | |
102 | are filled -> is filled | |
llvm/test/ObjectYAML/MachO/virtual_section.yaml | ||
178 | Why are these no longer CHECK-NEXT? Should you be testing the new stuff before it? |
Have you rebased recently? I feel like the content in yaml2macho.cpp may have moved to the ObjectYAML library recently.
I hadn't actually done that yet but just did so in rL368021
llvm/lib/Object/MachOObjectFile.cpp | ||
---|---|---|
1948 | This comes from the definition of struct section_64 [1]. [1]: https://github.com/opensource-apple/cctools/blob/master/include/mach-o/loader.h#L427-L428 | |
llvm/test/ObjectYAML/MachO/virtual_section.yaml | ||
178 | I removed the -NEXT because the newly added content fields were lengthy but it was bad idea. I've replaced them with regular expressions instead. | |
llvm/tools/obj2yaml/macho2yaml.cpp | ||
21 | I'd like to move this function into somewhere else instead of copying from MachOEmitter.cpp but I couldn't locate the appropriate place. Any suggestions? |
llvm/include/llvm/ObjectYAML/MachOYAML.h | ||
---|---|---|
203 | khm, to be honest, I would not place it here , these template specializations provide the static method "mapping" only. Somehow, I don't see where this method ("validate") is being used at the moment - maybe we can either delete it or hide it inside MachOYAML.cpp |
llvm/lib/ObjectYAML/MachOEmitter.cpp | ||
---|---|---|
322 ↗ | (On Diff #214064) | Could you add the missing trailing full stop whilst you're here, please? |
llvm/test/ObjectYAML/MachO/section_data.yaml | ||
45 | Delete the second "content". | |
llvm/tools/obj2yaml/macho2yaml.cpp | ||
21 | Two suggestions:
|
llvm/include/llvm/ObjectYAML/MachOYAML.h | ||
---|---|---|
203 | validate is an optional method in MappingTraits and it's implemented in MachOYAML.cpp. I think this should be here to validate obviously invalid input by YAMLTraits.h since it provides human-friendly error messages like: YAML:30:9: error: Section size must be greater than or equal to the content size - sectname: __data ^ yaml2obj: error: Failed to parse YAML input! | |
llvm/lib/ObjectYAML/MachOYAML.cpp | ||
294 | Here. | |
llvm/tools/obj2yaml/macho2yaml.cpp | ||
21 | Moved into the BinaryFormat/MachO.h. Thanks! |
LG, thanks for working on this!
llvm/include/llvm/ObjectYAML/MachOYAML.h | ||
---|---|---|
203 | fair enough |
khm, to be honest, I would not place it here , these template specializations provide the static method "mapping" only. Somehow, I don't see where this method ("validate") is being used at the moment - maybe we can either delete it or hide it inside MachOYAML.cpp