[PATCH] D44562: [ELF] Rework debug line parsing to use llvm::Error and callbacks (LLD-side)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: ELF/InputFiles.cpp:136
+                      [&](DebugLineError &Err) { warn(Err.message()); },
+                      [&](ErrorInfoBase &Err) { error(Err.message()); });
+  }
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > grimar wrote:
> > > > Is it useful to `warn` here? I think we only call this method on linkage error,
> > > > so anyways going to terminate the link.
> > > > With that, it seems easier to always error out here to simplify code/logic.
> > > I'm currently preserving behaviour (or fixing intended behaviour, at least), though I certainly see where you're coming from. In general we try to emit as many errors/warnings as we can before stopping, so I still think it's useful - it might help the user identify some other problem, for example.
> > > 
> > > I'm not bothered either way though, so if the consensus is to not emit the warning, I can easily change that.
> > I meant we always can `error` here instead of `warn` for all cases I think.
> As at this point, we are already in error state.
Right, but that would suggest to the user that fixing the malformed debug line is a requirement to get a working link, which it isn't. Perhaps another way to think about it is if we wanted to introduce a new warning that could use source information. We'd want to use this function too, but we shouldn't stop the link succeeding if we can't parse the line table.

I could actually make a case for making the second one a warning too, but I made that an error, because we don't actually ever expect it to be reported currently.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44562