[PATCH] D98437: [SystemZ][z/OS] Add GOFFObjectFile class support for HDR, ESD and END records

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

The pre-merge checks show build failures for Windows and Darwin. Please review them and fix as appropriate.



================
Comment at: llvm/include/llvm/Object/GOFF.h:125
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t Length = getPropertyModuleLength(Record);
----------------
yusra.syeda wrote:
> jhenderson wrote:
> > Same comment as above. Also, these `SmallString` diagnostic errors might imply you're missing an include, so other usages of this header might have issues.
> I'm not sure which include I'm missing. Shouldn't a missing include cause a build failure? 
The file that defines `SmallString` is `ADT/SmallString.h`. I went through the include tree and I couldn't find any reference to this header. A build error for a missing include may not happen, depending on how this file is included. For example, if your cpp file includes are:
```
#include "ADT/SmallString.h"
#include "Object/GOFF.h"
```
`SmallString` will be defined before GOFF.h is included, so the contents of GOFF.h will see it and not have a problem using it (remember, C and C++ includes are simply copy-and-paste of contents, so the final result is essentially equivalent to one big file).

If in the future, you created a source file that only included `Object/GOFF.h`, then you'd likely see the build error. At the moment, the only usage you have of `Object/GOFF.h` is the `Object/GOFFObjectFile.h` file, which includes other headers before this one. As such, I suspect that one of the headers that file includes will include SmallString.h, and therefore avoid the build error.

I therefore think you need to include `SmallString.h` in this file, to avoid confusing errors later.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98437/new/

https://reviews.llvm.org/D98437