[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
alexshap wrote:
> jhenderson wrote:
> > alexshap wrote:
> > > jhenderson wrote:
> > > > jhenderson wrote:
> > > > > plotfi wrote:
> > > > > > jhenderson wrote:
> > > > > > > You can get rid of this from the header by forward-declaring the DebugCompressionType enum.
> > > > > > You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).
> > > > > You aren't using them in the header file, as far as I can see. Move the include into the .cpp if it is needed there. Same goes for Compression.h.
> > > > You should still forward declare the enum in the header and include MCTargetOptions.h in the cpp file.
> > > @jhenderson - khm, if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h. Just curious - what are the benefits of this / are there any pointers to the llvm style guide or any other docs suggesting this ? that would look kinda broken (one header (Object.h) implicitly depends on another one and can not be included without causing errors into an empty .cpp). 
> > > if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h.
> > Not true. This is an `enum class` which can be forward declared (defaults to `int` size unless explicitly stated). See https://en.cppreference.com/w/cpp/language/enum. Only clients of the code that actually want to access an enumeration value need to include the header.
> > 
> > It's bad practice to include more headers than needed in general, due to the increased compile time cost, and also creation of implicit dependencies that later could result in broken builds.
> @jhenderson 
> 
> 1. Not true.
> alexshap-mbp:~ alexshap$ cat main.cpp
> enum F;
> int main() {
>   return sizeof(F);
> }
> alexshap-mbp:~ alexshap$ clang -std=c++11 main.cpp -o main.exe
> main.cpp:1:6: error: ISO C++ forbids forward references to 'enum' types
> enum F;
>      ^
> main.cpp:3:10: error: invalid application of 'sizeof' to an incomplete type 'F'
>   return sizeof(F);
>          ^     ~~~
> main.cpp:1:6: note: forward declaration of 'F'
> enum F;
>      ^
> 2 errors generated.
> 
> 2. Nobody suggested including more headers than needed,  my point was different,
> it seems like it would be strange if whenever someone wants to include <vector> and use smth from it (for example, sizeof(std::vector<int>)) he has to include <map> as well.
> 
> 
I see, "enum class" is different, in this case it can be forward declared, I didn't notice that.
Yeah, in this case it's appropriate and @jhenderson is right.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678