This is an archive of the discontinued LLVM Phabricator instance.

[Coding style change][lld] Rename variables for non-ELF ports.
ClosedPublic

Authored by ruiu on Jul 10 2019, 2:15 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.Jul 10 2019, 2:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar added inline comments.Jul 10 2019, 2:26 AM
lld/COFF/Chunks.h
102 ↗(On Diff #208899)

Should it be osIdx perhaps?

lld/COFF/DebugTypes.cpp
263 ↗(On Diff #208899)

pdbFile?

lld/COFF/Driver.h
124 ↗(On Diff #208899)

mbRef?

lld/COFF/InputFiles.cpp
139 ↗(On Diff #208899)

coffObj?

lld/wasm/SymbolTable.cpp
520 ↗(On Diff #208899)

It looks unformatted. Perhaps you could apply the clang format at the same time?

lld/wasm/SyntheticSections.h
289 ↗(On Diff #208899)

sDKs looks strange. sdks?

ruiu updated this revision to Diff 208901.Jul 10 2019, 2:30 AM
  • fix odd names such as cOFFSym
ruiu updated this revision to Diff 208902.Jul 10 2019, 2:33 AM
ruiu marked an inline comment as done.
  • address review comments
ruiu marked 4 inline comments as done.Jul 10 2019, 2:34 AM
ruiu added inline comments.
lld/COFF/Driver.h
124 ↗(On Diff #208899)

I think this is fine.

lld/wasm/SymbolTable.cpp
520 ↗(On Diff #208899)

It is intentional that I did not run clang-format, as some code was intended to not be formatted using clang-format. I don't judge whether that expectation is good or not, but I wanted to avoid the problem by not running clang-format to this patch. If we do, we'd virtually apply clang-format to the entire code base. That might be a good thing but needs a separate discussion.

Many variables in assert and LLVM_DEBUG are not processed. You probably need to apply the tool on a debug (or -DLLVM_ENABLE_ASSERTIONS=on) build .

lld/wasm/Symbols.cpp
96 ↗(On Diff #208902)

outputSymbolIndex

101 ↗(On Diff #208902)

name index

103 ↗(On Diff #208902)

outputSymbolIndex

108 ↗(On Diff #208902)

name index

109 ↗(On Diff #208902)

gotIndex

191 ↗(On Diff #208902)

assert(tableIndex == INVALID_INDEX);

lld/wasm/Symbols.h
115 ↗(On Diff #208902)

gotIndex

ruiu updated this revision to Diff 209117.Jul 10 2019, 8:21 PM
  • enable assertion so that expressions in assert() are properly converted
ruiu updated this revision to Diff 209118.Jul 10 2019, 8:50 PM
  • skimmed through the patch again to find and corrected odd conversion such as gCRoot
MaskRay accepted this revision.Jul 10 2019, 10:31 PM
MaskRay added inline comments.
lld/COFF/Driver.cpp
126 ↗(On Diff #209118)

This tool doesn't handle argument comments like /*FileSize*/ .

But I think it is fine to update them in another commit.

This revision is now accepted and ready to land.Jul 10 2019, 10:31 PM
This revision was automatically updated to reflect the committed changes.
jrtc27 added a subscriber: jrtc27.Sep 10 2019, 11:05 AM

I'm trying to merge this into our fork. The previous ELF patch was fine, but for this commit I'm having issues because the tool in D64123 doesn't produce this exact result, and so running it on our tree produces a ton of conflicts. Did you use a modified clang-llvm-rename for this (and if so could you please upload it), or are all the discrepancies changes you made by hand afterwards (or with a separate script you would be able to provide)?