Details
- Reviewers
majnemer timurrrr jhenderson
Diff Detail
Event Timeline
Hi Jeff,
I'm currently traveling, so can't look at your patch yet. I'll try to take
a look in the next couple is weeks!
Tim
вт, 28 окт. 2014, 11:36, Jeff Muizelaar <jmuizelaar@mozilla.com>:
Hi timurrrr,
Files:
include/llvm/Support/COFF.h test/tools/llvm-readobj/codeview-linetables.test tools/llvm-readobj/COFFDumper.cpp
Sorry for missing this, I was travelling and later overwhelmed by a backlog of breakages... Thanks for reminding!
General comments:
- Are you working on FPO table generation in LLVM?
- If no, why do you need to dump this?
- Either way, do you need to dump all the fields of the FPO subsection or just a subset suffice?
- You probably need to add more test cases specifically to cover more FPO-related corner cases rather than modify existing tests.
Adding David who knows COFF better than I do.
include/llvm/Support/COFF.h | ||
---|---|---|
678 | This constant is not used anywhere in the tests. | |
test/tools/llvm-readobj/codeview-linetables.test | ||
119 | Is this format documented somewhere? | |
123 | Looks like the same ProgramStr is generated for all these test functions. | |
tools/llvm-readobj/COFFDumper.cpp | ||
521 | I think you've forgotten to update the comment. | |
537 | nit: offset | |
582 | the body of this for loop is getting hard to read. Maybe it's time to split it into a few subroutines? |
test/tools/llvm-readobj/codeview-linetables.test | ||
---|---|---|
119 | The format is not documented anywhere. I reconstructed it from the wine dbghelp implementation and https://msdn.microsoft.com/en-us/library/zxsd009h.aspx. | |
123 | I can try to get more diverse set of inputs to test the output with. | |
tools/llvm-readobj/COFFDumper.cpp | ||
582 | Sure. |
This constant is not used anywhere in the tests.