[Lldb-commits] [PATCH] D132307: [lldb] Switch RegularExpression from llvm::Regex to std::regex

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
Comment at: lldb/source/Utility/CMakeLists.txt:29
+  PROPERTIES COMPILE_OPTIONS
+  "-fcxx-exceptions"
+)
----------------
kastiglione wrote:
> JDevlieghere wrote:
> > kastiglione wrote:
> > > the `std::regex` constructor throws `std::regex_error` if the pattern is invalid. For this reason, exceptions are enabled for this one file.
> > What happens when exceptions are disabled? What does it mean to have this enabled for a single file? I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions. 
> > What happens when exceptions are disabled?
> 
> This cmake config enables exceptions for this one file, independent of `LLVM_ENABLE_EH`. No other source files will be allowed to catch or throw exceptions.
> 
> > What does it mean to have this enabled for a single file?
> 
> It means this file can compile with a try/catch, and that inside this file, exceptions can be caught.
> 
> > I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions.
> 
> llvm has `LLVM_ENABLE_EH` which allows llvm to be built with exceptions support enabled. Similarly, `LLVM_ENABLE_RTTI` allows RTTI to be enabled. It seems that both are disabled as a default, but not as a hard requirement.
> 
> I wondered if enabling RTTI would needed for exceptions, but at least for this code, the answer is no. The `catch (const std::regex_error &e)` block is exercised by `TestBreakpointRegexError.py`, so we know this code can and does catch an exception of that type, and accesses the error's member functions.
> 
> 
What makes me believe this use of exceptions is ok is that the API boundary continues to be exception free, callers continue to be exception disabled. Only the internal implementation, knows about exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132307