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

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda marked 6 inline comments as done.
yusra.syeda added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFF.h:125
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t Length = getPropertyModuleLength(Record);
----------------
jhenderson wrote:
> 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.
> 
Thanks for the explanation, I've added this include.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:349-350
+  }
+  return createStringError(llvm::errc::invalid_argument,
+                           "no symbol section found");
+}
----------------
jhenderson wrote:
> jhenderson wrote:
> > This message could be improved, I believe. If I understand it correctly, this error is trying to say that a symbol's section is not found, rather than that a "symbol section" is not found. I think you should record more information in the message, i.e. which symbol is missing its section, and what should identify that section (index/name/ID or whatever - I don't really follow the actual logic).
> Ping this comment? (It was referring to the "no symbol section found" error message).
> 
> Tip: if you have not yet addressed a comment, but have plans to do so in a future update, say so when you update the patch.
This error message has been updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437