Small update to Expected<StringRef>

[sidneym]

Summary

I think this was missed when the original change was made. Should have used NameOrErr instead of SecIOrErr.

[MaskRay]

LGTM. Did you run into this issue with a binary? If yes, can you simplify it and add a test?

[grimar]

Blame list shows it was my change. LGTM. I'll take care about the test.
Thanks!

[grimar]

Test case patch is: D67002 (it depends on a one more patch for yaml2obj).

[dblaikie]

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

[MaskRay]

In D66976#1658669, @dblaikie wrote:

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

rL370669 added a test

[dblaikie]

In D66976#1658737, @MaskRay wrote:

In D66976#1658669, @dblaikie wrote:

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

rL370669 added a test

That doesn't seem to test this patch, though - if I revert this patch no tests fail.

[grimar]

In D66976#1659909, @dblaikie wrote:

In D66976#1658737, @MaskRay wrote:

In D66976#1658669, @dblaikie wrote:

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

rL370669 added a test

That doesn't seem to test this patch, though - if I revert this patch no tests fail.

Did you set LLVM_ENABLE_ABI_BREAKING_CHECKS to 1?

Before this patch NameOrErr was not consumed/ignored by consumeError.
If you revert it and run the test from rL370669, you should get a
"Expected<T> must be checked before access or destruction" failture.

[dblaikie]

In D66976#1660407, @grimar wrote:

In D66976#1659909, @dblaikie wrote:

In D66976#1658737, @MaskRay wrote:

In D66976#1658669, @dblaikie wrote:

Any chance of a test case? This looks like it fixed a bug/difference in behavior.

rL370669 added a test

That doesn't seem to test this patch, though - if I revert this patch no tests fail.

Did you set LLVM_ENABLE_ABI_BREAKING_CHECKS to 1?

Before this patch NameOrErr was not consumed/ignored by consumeError.
If you revert it and run the test from rL370669, you should get a
"Expected<T> must be checked before access or destruction" failture.

Must've screwed it up somehow - maybe I used lit to run the test directly without rebuilding llvm-nm.

In any case - got it now - sorry for the runaround!
(also, can be helpful in the future to note that a patch is fixing a buildbot failure/existing test suite failure - otherwise it's harder to tell if it's missing test coverage)

[grimar]

In D66976#1661686, @dblaikie wrote:

(also, can be helpful in the future to note that a patch is fixing a buildbot failure/existing test suite failure - otherwise it's harder to tell if it's missing test coverage)

Sure. But in this case this patch was landed first, since it fixed an obvious misspeling. (i.e. there was bo BB failture or existent test).
Writing the test was a bit complicated since it requred one more patch for yaml2obj
to produce the binary broken in a specific way. So it was landed without the test which was added in rL370669 after rL370633 (patch for yaml2obj) was landed.
And the description for rL370669 contans the information about what it is for :)

llvm/trunk/tools/llvm-nm/llvm-nm.cpp

else {
       Expected<elf_section_iterator> SecIOrErr = SymI->getSection();
       if (!SecIOrErr) {
         consumeError(SecIOrErr.takeError());
         return '?';
       }
 
       Expected<StringRef> NameOrErr = (*SecIOrErr)->getName();
       if (!NameOrErr) {
-        consumeError(SecIOrErr.takeError());
+        consumeError(NameOrErr.takeError());
         return '?';
       }
       SecName = *NameOrErr;
     }
   }
 
   if ((Symflags & object::SymbolRef::SF_Weak) && !isa<MachOObjectFile>(Obj)) {
     char Ret = isObject(Obj, I) ? 'v' : 'w';