[PATCH] D43788: Merge {COFF, ELF}/Strings.cpp to Common/Strings.cpp.

[Rafael Avila de Espindola via llvm-commits]

Rui Ueyama <ruiu at google.com> writes:

> On Mon, Feb 26, 2018 at 4:27 PM Rafael Avila de Espindola <
> rafael.espindola at gmail.com> wrote:
>
>> Rui Ueyama via Phabricator <reviews at reviews.llvm.org> writes:
>>
>> > Index: lld/include/lld/Common/Strings.h
>> > ===================================================================
>> > --- lld/include/lld/Common/Strings.h
>> > +++ lld/include/lld/Common/Strings.h
>> > @@ -10,14 +10,71 @@
>> >  #ifndef LLD_STRINGS_H
>> >  #define LLD_STRINGS_H
>> >
>> > +#include "llvm/ADT/ArrayRef.h"
>> >  #include "llvm/ADT/Optional.h"
>> >  #include "llvm/ADT/StringRef.h"
>> > +#include "llvm/Support/GlobPattern.h"
>> >  #include <string>
>> > +#include <vector>
>> > +
>> > +namespace llvm {
>> > +class GlobPattern;
>> > +}
>>
>> Why do you need both the include and the forward declaration?
>>
>>
>> >  namespace lld {
>> >  // Returns a demangled C++ symbol name. If Name is not a mangled
>> >  // name, it returns Optional::None.
>> >  llvm::Optional<std::string> demangleItanium(llvm::StringRef Name);
>> > +llvm::Optional<std::string> demangleMSVC(llvm::StringRef S);
>>
>> Should this be in Common? When would any linker other than COFF need
>> this?
>>
>
> Technically it is used only by COFF, but since these functions are so small
> that I think that putting everything in one file doesn't hurt. But don't
> you like it?

Fair enough.

LGTM with the two other nits fixed.

Cheers,
Rafael