This patch is a breakdown of https://reviews.llvm.org/D89071, adding support for only the HDR, ESD and END GOFF records.
No need for braces for single-line if.
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).
Nit: this could be EXPECT_THAT_EXPECTED. The test doesn't need to abort, if this check fails.
This can be EXPECT_THAT_EXPECTED, right? There's no need to stop this test if it has a problem in this case, I'd think.
Ping this comment?
Please keep the old naming scheme. If you aren't familiar with it, check out the LLVM coding guidelines (https://llvm.org/docs/CodingStandards.html), which includes documentation on naming functions and variables (functions lowerCamelCase, variables UpperCamelCase being the gist).
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.
Have you built this code? This error looks like it would break the build...
Were you using '\0' (with quotes)? If so, getting an error doesn't seem right. The return type of (char)0x00 is identical to '\0'. What is the error that you are getting? What compiler version are you using?
Similar to my other comments, this and the following error messages don't really provide enough context for a user to addresse them. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages for explanation and examples.
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.
Same as my other comments.
I removed the setters from this file as they are not required for this patch. The need for this change no longer applies.
I'm not sure which include I'm missing. Shouldn't a missing include cause a build failure?
Thanks, I will add more details to this error message and the others you pointed out.
The pre-merge checks show build failures for Windows and Darwin. Please review them and fix as appropriate.
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.
Is there an index/ID/offset you could use in these messages? It will help users find the invalid record (since it may not be easy to identify which record has the specified type, if things are broken). Take a look at how llvm-readobj reports errors in ELF objects for some good examples.
Same goes for your other error messages.
Please ensure you build and test your code before uploading a patch - this will cause a build failure.
There are a number of these clang-tidy complaints, similar to how there were those SmallString complaints before. Would it make sense to move the body of this and similar functions into a cpp? That would avoid needing to include Error.h in this file directly.
inconsistent constexpr/const. You can use constexpr everywhere.
the prevailing code style doesn't insert a blank line between nested namespaces
delete user-defined constructor which does nothing. ditto elsewhere in the file
!= is more common