[PATCH] D66089: [llvm/Object] - Convert SectionRef::getName() to return Expected<>

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/Object/ELFObjectFile.h:468
+  if (Expected<section_iterator> SecOrErr = getSymbolSection(Sym)) {
+    consumeError(Name.takeError());
+    return (*SecOrErr)->getName();
----------------
JDevlieghere wrote:
> What if the Expected is in the success state, but the StringRef is empty?
Thanks! I fixed this case.


================
Comment at: include/llvm/Object/ObjectFile.h:438
+inline Expected<StringRef> SectionRef::getName() const {
   Expected<StringRef> NameOrErr = OwningObject->getSectionName(SectionPimpl);
   if (!NameOrErr)
----------------
sbc100 wrote:
> Can you return the result of `getSectionName` here?
Done, thanks!


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1512
+      else
+        consumeError(NameOrErr.takeError());
+
----------------
sbc100 wrote:
> Add TODOs here and below to handle errors?
I did not add TODOs here or in another places intentionally.
It feels that many places lack the error checking and needs adding "TODOs",
but I am not sure which of them really need it (i.e. sometimes consuming errors is OK).
What I was thinking about is that ideally
each place where we have a `consumeError` in LLVM should have a
comment clarifying everything is OK and its using was intentional. If not -  then it is
an implicit "TODO" that we need to check.

I.e. I would like to not to add "TODOs" in this patch if you and others are OK with that.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h:287
       const object::SectionRef &Section = SectionPair.first;
-      StringRef Name;
-      if (auto EC = Section.getName(Name))
-        return errorCodeToError(EC);
+      Expected<StringRef> ErrorOrName = Section.getName();
+      if (!ErrorOrName)
----------------
sbc100 wrote:
> You use `ErrorOrName` here but `NameOrErr` here.  Presumably we should prefer one or at least be consistent?
Done.


================
Comment at: lib/Object/ELFObjectFile.cpp:386
   for (const SectionRef &Section : sections()) {
-    StringRef Name;
-    if (Section.getName(Name))
+    auto NameOrErr = Section.getName();
+    if (!NameOrErr) {
----------------
sbc100 wrote:
> In most other places you use `Expected<StringRef>` rather than auto.   Is there a reason for auto here and not elsewhere?
No. Sometimes LLVM code use `auto` for expressing the `Expected<>` return type.
I would prefer to see an explicit types in such situations. Fixed.


================
Comment at: tools/llvm-cov/TestingSupport.cpp:52
   for (const auto &Section : OF->sections()) {
-    StringRef Name;
-    if (Section.getName(Name))
+    Expected<StringRef> NameOrErr = Section.getName();
+    if (!NameOrErr) {
----------------
MaskRay wrote:
> Something like:
> 
>       if (Expected<StringRef> NameOrErr = Section.getName())
>         SectName = *NameOrErr;
>       else {
>         consumeError(NameOrErr.takeError());
>         return 1;
>      }
This is one line longer, but OK.


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

https://reviews.llvm.org/D66089