This is an archive of the discontinued LLVM Phabricator instance.

[flang][nfc] Add missing headers in TypeConverter.h
ClosedPublic

Authored by awarzynski on Nov 10 2021, 7:30 AM.

Details

Summary

These missing header files wouldn't cause build failures as TypeConverter.h
is currently only included in CodeGen.cpp and that file includes everything
that's needed in TypeConverter.h. However, as per [1], "(...)
include all of the header files that you are using ".

I've also added missing namespace qualifiers in 2 places.

[1] https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible

Diff Detail

Event Timeline

awarzynski created this revision.Nov 10 2021, 7:30 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mehdi_amini. · View Herald Transcript
awarzynski requested review of this revision.Nov 10 2021, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 7:30 AM

We should also probably clean up CodeGen.cpp while adding the headers here.

flang/lib/Optimizer/CodeGen/TypeConverter.h
23

I don't think this one is needed.

Remove #include "llvm/ADT/StringMap.h"

We should also probably clean up CodeGen.cpp while adding the headers here.

I can trim the list of #includes in CodeGen.cpp, but there's a danger that I remove something that *is* used (but everything will build fine as it will be included with some other header file). Or did you you have something else in mind?

We should also probably clean up CodeGen.cpp while adding the headers here.

I can trim the list of #includes in CodeGen.cpp, but there's a danger that I remove something that *is* used (but everything will build fine as it will be included with some other header file). Or did you you have something else in mind?

That's what I had in mind. Some includes are in CodeGen.cpp only for TypeConverter.h

flang/lib/Optimizer/CodeGen/TypeConverter.h
251

Remove #includes from CodeGen.cpp that belong in TypeConverter.h

Address the remaining comments

clementval accepted this revision.Nov 11 2021, 1:42 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 11 2021, 1:42 AM

FYI: I've updated the summary/commit msg before merging this in. The original one (at the top) is a bit out-of-sync after the updates.