This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Including <string> in llvm-cxxdump/Error.cpp
ClosedPublic

Authored by qiongsiwu1 on Oct 7 2021, 1:53 PM.

Details

Summary

A recent commit removed <string> from ErrorHandling.h. The removal caused <string> to be no longer included for llvm/tools/llvm-cxxdump/Error.cpp which uses the string type.

This patch adds <string> to llvm/tools/llvm-cxxdump/Error.cpp.

Diff Detail

Event Timeline

qiongsiwu1 requested review of this revision.Oct 7 2021, 1:53 PM
qiongsiwu1 created this revision.
qiongsiwu1 edited the summary of this revision. (Show Details)Oct 7 2021, 1:57 PM
jsji accepted this revision.Oct 7 2021, 2:37 PM

LGTM.

This revision is now accepted and ready to land.Oct 7 2021, 2:37 PM
This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Oct 8 2021, 1:13 AM

@qiongsiwu1 I ran check-llvm on windows, wsl and linux boxes and didn't hit this - does this mean llvm-cxxdump isn't built/tested by default at all?

@qiongsiwu1 I ran check-llvm on windows, wsl and linux boxes and didn't hit this - does this mean llvm-cxxdump isn't built/tested by default at all?

Thanks for looking into this! llvm-cxxdump builds on Linux without explicitly including the header, at least during our testing. For Linux on Power, it happens to work for us due to the build compiler including the header through other means as a side effect (Specifically, the utility header including cstdlib). The build failed on AIX because the libc++ on the platform not having such side effects.

@qiongsiwu1 I ran check-llvm on windows, wsl and linux boxes and didn't hit this - does this mean llvm-cxxdump isn't built/tested by default at all?

Thanks for looking into this! llvm-cxxdump builds on Linux without explicitly including the header, at least during our testing. For Linux on Power, it happens to work for us due to the build compiler including the header through other means as a side effect (Specifically, the utility header including cstdlib). The build failed on AIX because the libc++ on the platform not having such side effects.

Thanks for the clarification - the lack of buildbot failures suggested I'd gotten it mostly right - implicit dependencies will always hit in unexpected places of course.....

@qiongsiwu1 I ran check-llvm on windows, wsl and linux boxes and didn't hit this - does this mean llvm-cxxdump isn't built/tested by default at all?

Thanks for looking into this! llvm-cxxdump builds on Linux without explicitly including the header, at least during our testing. For Linux on Power, it happens to work for us due to the build compiler including the header through other means as a side effect (Specifically, the utility header including cstdlib). The build failed on AIX because the libc++ on the platform not having such side effects.

Thanks for the clarification - the lack of buildbot failures suggested I'd gotten it mostly right - implicit dependencies will always hit in unexpected places of course.....

Not a problem! This was rather unexpected to us as well.