This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Move MinGW specific functions/classes to a separate file. NFC.
ClosedPublic

Authored by mstorsjo on Oct 18 2017, 12:48 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 18 2017, 12:48 PM
ruiu accepted this revision.Oct 18 2017, 3:14 PM

LLVM with these style changes.

COFF/MinGW.cpp
19–20 ↗(On Diff #119517)

Instead of enclosing the entire file with namespace, add coff:: to non-static functions just like we do in ELF.

COFF/MinGW.h
21 ↗(On Diff #119517)

Instead of doing this, please include LLVM.h just like other headers in lld do.

This revision is now accepted and ready to land.Oct 18 2017, 3:14 PM
mstorsjo marked an inline comment as done.Oct 18 2017, 11:48 PM
mstorsjo added inline comments.
COFF/MinGW.h
21 ↗(On Diff #119517)

LLVM.h doesn't include or forward-declare StringSet, but if I add it there, I can get this working (as long as any caller trying to instantiate the AutoExporter class has pulled in the proper definition of StringSet).

mstorsjo updated this revision to Diff 119548.Oct 18 2017, 11:49 PM

Removed the whole-file namespace in MinGW.cpp, removed manual includes of ADT types in MinGW.h and added a forward declaration to LLVM.h.

ruiu accepted this revision.Oct 19 2017, 9:12 AM

LGTM

This revision was automatically updated to reflect the committed changes.

FWIW, the StringSet forward declaration turned out to fail on clang while it worked fine when built with GCC, see e.g. http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/11694/steps/build_Lld/logs/stdio (https://godbolt.org/g/czUQZ8 for a simplified example of the issue). I did a quick fix by removing the forward declaration again and using the fully qualified llvm::StringSet in MinGW.h (instead of the using statement that you weren't so fond of earlier in review).