[PATCH] D103089: [Debug-Info][CodeView] Fix GUID string generation for MSVC generated objects.

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp:37
+void checkData(GuidData &Data) {
+  struct MSGuid {
+    support::ulittle32_t Data0;
----------------
amccarth wrote:
> I see why it's done, but it's a bit confusing that this definition of MSGuid is different from the one in the formatting code.  In fact, this one has 18 bytes of data and may contain padding.
> 
> If you were to move this definition down to the one place it's used, it might not be as confusing.
> I see why it's done, but it's a bit confusing that this definition of MSGuid is different from the one in the formatting code.  In fact, this one has 18 bytes of data and may contain padding.
> 
To avoid confusion between those definitions, I have renamed the one used in the tests.

> If you were to move this definition down to the one place it's used, it might not be as confusing.

Moved the definition down to the place where is used.



================
Comment at: llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp:82
+
+TEST(GUIDFormatTest, Data_00) {
+  GuidData Data = {
----------------
amccarth wrote:
> It's not clear to my why we need 16 of these tests.  Is this overkill?
> 
> The names Data_xx don't really clue me in as to what's going on in each test set.  I see the "Shifting xx" comments, but that still doesn't quite do it for me.
> 
> From the comments, I expected a set of tests for Variant 1 (UUIDs) and one for Variant 2 (MS/COM-style GUIDs).
> It's not clear to my why we need 16 of these tests.  Is this overkill?
> 
The idea was to shift a zero value across all components. But as you pointed it out, it can be overkill. I have removed most of the tests. Keeping just one that covers the corner cases.

> The names Data_xx don't really clue me in as to what's going on in each test set.  I see the "Shifting xx" comments, but that still doesn't quite do it for me.

Rename the test as 'ValidateFormat'.

> From the comments, I expected a set of tests for Variant 1 (UUIDs) and one for Variant 2 (MS/COM-style GUIDs).

The test are intended only for the Variant 2. Added a explanatory note.



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

https://reviews.llvm.org/D103089