[PATCH] D41146: [DWARF] DWARF v5: Rework of string offsets table reader

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:113
+              if (!L)
+                return true; // Invalid L should always precede R.
+              if (R)
----------------
probinson wrote:
> aprantl wrote:
> > Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?
> I think the most compact code would be
> ```
> if (L && R) return L->base < R->base;
> return R;
> ```
> if both are valid, order depends on the base.
> if L is invalid and R valid, L sorts before R.
> if L is valid and R invalid, R sorts before L.
> if both are invalid, they are equivalent (test(R,L) == test(L,R)).
> 
> Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?

That's true. Several invalid contributions in the vector will probably end up in different locations when the initial order is different, but all that matters is that all the invalid ones end up at the beginning. It would only matter if we tried to be more precise about how and why they are invalid (bad start vs. invalid length etc.), but this patch doesn't try to do that. It's not clear to me if it would be worth it.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:113
+              if (!L)
+                return true; // Invalid L should always precede R.
+              if (R)
----------------
wolfgangp wrote:
> probinson wrote:
> > aprantl wrote:
> > > Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?
> > I think the most compact code would be
> > ```
> > if (L && R) return L->base < R->base;
> > return R;
> > ```
> > if both are valid, order depends on the base.
> > if L is invalid and R valid, L sorts before R.
> > if L is valid and R invalid, R sorts before L.
> > if both are invalid, they are equivalent (test(R,L) == test(L,R)).
> > 
> > Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?
> 
> That's true. Several invalid contributions in the vector will probably end up in different locations when the initial order is different, but all that matters is that all the invalid ones end up at the beginning. It would only matter if we tried to be more precise about how and why they are invalid (bad start vs. invalid length etc.), but this patch doesn't try to do that. It's not clear to me if it would be worth it.
> I think the most compact code would be 

Sleek! I can put that in, if you guys prefer.



https://reviews.llvm.org/D41146