Page MenuHomePhabricator

Remove tautological #ifdefs
ClosedPublic

Authored by aprantl on Mar 1 2019, 10:33 AM.

Details

Summary

These conditions appear to always be true.

@clayborg do you remember the reason why these exist?

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Mar 1 2019, 10:33 AM
clayborg requested changes to this revision.Mar 1 2019, 11:26 AM

LLDB_CONFIGURATION_BUILD_AND_INTEGRATION doesn't have them. So you can change these to

#if !defined(LLDB_CONFIGURATION_BUILD_AND_INTEGRATION)
This revision now requires changes to proceed.Mar 1 2019, 11:26 AM

So it affects Apple directly!

I think we mostly wanted to catch these in the test suite. Probably best to convert to lldbassert

But I'm sure that LLDB_CONFIGURATION_BUILD_AND_INTEGRATION also defines NDEBUG so the assert should be a noop, right?

This doesn't apply to DieStack since it isn't an assert, but DieStack is dead code anyway...

zturner added a subscriber: zturner.Mar 1 2019, 2:41 PM

Yea it would be nice if we could remove all of the LLDB_CONFIGURATION_xxx macros and just use either the LLVM ones or standard ones such as NDEBUG

Yea it would be nice if we could remove all of the LLDB_CONFIGURATION_xxx macros and just use either the LLVM ones or standard ones such as NDEBUG

I generally agree with this, but we need to decide what to do on a case-by-case basis. So far I found four categories of LLDB_CONFIGURATION_DEBUG

  1. assertions that should just be "assert"
  2. expensive checks that should be guarded by LLVM_ENABLE_EXPENSIVE_CHECKS instead
  3. Consistency checks that assume that the debug info is 100% accurate/complete and that may fail in the real world
  4. additional logging

there may be other cases that I haven't found yet.

Also I should note that we can only use LLVM defines that are provided by the Xcode project until we reach the point where Apple builds LLDB with CMake and the CMake-generated Xcode file is good enough to replace the hand-written one. This is a transitional restriction, but it's a longer project...

jingham added a subscriber: jingham.Mar 1 2019, 3:40 PM

Yea it would be nice if we could remove all of the LLDB_CONFIGURATION_xxx macros and just use either the LLVM ones or standard ones such as NDEBUG

I generally agree with this, but we need to decide what to do on a case-by-case basis. So far I found four categories of LLDB_CONFIGURATION_DEBUG

  1. assertions that should just be "assert"
  2. expensive checks that should be guarded by LLVM_ENABLE_EXPENSIVE_CHECKS instead
  3. Consistency checks that assume that the debug info is 100% accurate/complete and that may fail in the real world
  4. additional logging

    there may be other cases that I haven't found yet.

Additional logging should really go through the Verbose version of whatever log channel is appropriate. The additional code won't get run in the normal logging case, so there's no reason to throw it away.

aprantl updated this revision to Diff 189003.Mar 1 2019, 4:32 PM

Removed DIEStack from this patch it distracts.

Ping. As far as I can tell, now this patch should be entirely NFC.

davide accepted this revision.Mar 4 2019, 4:05 PM
davide added a subscriber: davide.

LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 5 2019, 3:53 PM
Closed by commit rL355457: Remove tautological #ifdefs (NFC) (authored by adrian, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 3:53 PM