[Lldb-commits] [PATCH] D150639: [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

In D150639#4346649 <https://reviews.llvm.org/D150639#4346649>, @JDevlieghere wrote:

> In D150639#4346009 <https://reviews.llvm.org/D150639#4346009>, @rupprecht wrote:
>
>> +1 to being surprised this is not already the case
>>
>> Some other places should be updated after this, e.g. lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:
>>
>>   #if defined(LLDB_CONFIGURATION_DEBUG)
>>     // We assert that we have to module lock by trying to acquire the lock from a
>>     // different thread. Note that we must abort if the result is true to
>>     // guarantee correctness.
>>     assert(std::async(
>>                std::launch::async,
>>                [this] {
>>                  return this->GetModuleMutex().try_lock();
>>                }).get() == false &&
>>            "Module is not locked");
>>   #endif
>>
>> -> just wrap it in lldbassert
>
> This code actually has a comment right above it explaining that it's too expensive to do in release builds, so this isn't a good candidate for `lldbassert`. But this logic can be improved: it duplicates checking the `LLDB_CONFIGURATION_DEBUG` in both `ASSERT_MODULE_LOCK` and `AssertModuleLock` and should use `NDEBUG` too. I'll address that in a separate patch.

WDYT about just replacing `LLDB_CONFIGURATION_DEBUG` with LLVM's `EXPENSIVE_CHECKS` in this case?

>> In D150639#4344695 <https://reviews.llvm.org/D150639#4344695>, @JDevlieghere wrote:
>>
>>> This patch also uses `__FILE_NAME__` (Clang-specific extension that functions similar to __FILE__ but only renders the last path component (the filename) instead of an invocation dependent full path to that file.) when building with clang.
>>
>> Can you put this comment in `LLDBAssert.h` itself?
>
> Good idea.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150639