Page MenuHomePhabricator

LowerTypeTests: Rename local functions to avoid collisions with identically named functions in ThinLTO modules.
ClosedPublic

Authored by pcc on Sep 23 2019, 6:07 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Sep 23 2019, 6:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 6:07 PM
dexonsmith added inline comments.Sep 23 2019, 6:12 PM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1890–1896 ↗(On Diff #221452)

This looks like it could collide with an existing function. Clang handles this in a few places by looping until it finds an available name.

pcc marked an inline comment as done.Sep 23 2019, 6:28 PM
pcc added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1890–1896 ↗(On Diff #221452)

I was under the impression that this was already handled by setName (see http://llvm-cs.pcc.me.uk/lib/IR/Value.cpp#282 -> http://llvm-cs.pcc.me.uk/lib/IR/ValueSymbolTable.cpp#112 -> http://llvm-cs.pcc.me.uk/lib/IR/ValueSymbolTable.cpp#61), although it may be worth adding a test for this case.

dexonsmith added inline comments.Sep 23 2019, 6:49 PM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1890–1896 ↗(On Diff #221452)

Ah, you're right. A test for that might be nice, but a comment might also be enough given that it's likely tested elsewhere.

pcc edited the summary of this revision. (Show Details)Sep 24 2019, 10:00 AM
pcc updated this revision to Diff 221557.Sep 24 2019, 10:08 AM
  • Add comment
pcc marked an inline comment as done.Sep 24 2019, 10:08 AM
pcc added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1890–1896 ↗(On Diff #221452)

Okay, I've added a comment here.

pcc added a comment.Oct 3 2019, 3:47 PM

eugenis: Ping.

eugenis accepted this revision.Oct 3 2019, 4:05 PM

LGTM

This revision is now accepted and ready to land.Oct 3 2019, 4:05 PM
This revision was automatically updated to reflect the committed changes.