Re: [Libreoffice-commits] core.git: convert DBG_ASSERT in vcl

On 06/17/2016 08:39 AM, Noel Grandin wrote:
> New commits:
> commit 9c79945ca62b18213728cdd23d9f390304aee1de
> Author: Noel Grandin <noelgrandin@gmail.com>
> Date:   Sun Jun 12 20:11:20 2016 +0200
>
>     convert DBG_ASSERT in vcl
>
>     Change-Id: I732fb1a789f90ca7a7f393cc41a6afe84fecf3d3
>     Reviewed-on: https://gerrit.libreoffice.org/26200
>     Tested-by: Jenkins <ci@libreoffice.org>
>     Reviewed-by: Noel Grandin <noelgrandin@gmail.com>
>
> diff --git a/vcl/osx/salframe.cxx b/vcl/osx/salframe.cxx
> index dae0aef..b5a1499 100644
> --- a/vcl/osx/salframe.cxx
> +++ b/vcl/osx/salframe.cxx
> @@ -107,7 +107,7 @@ AquaSalFrame::~AquaSalFrame()
>      pSalData->maFrameCheck.erase( this );
>      pSalData->maPresentationFrames.remove( this );
>
> -    DBG_ASSERT( this != s_pCaptureFrame, "capture frame destroyed" );
> +    SAL_WARN_IF( this == s_pCaptureFrame, "vcl", "capture frame destroyed" );
>      if( this == s_pCaptureFrame )
>          s_pCaptureFrame = nullptr;
[...]

Such wholesale replacement of (potentially ambiguous) DBG_ASSERT etc. 
with SAL_WARN is what I always feared.  The main problem with the old 
macros is their unclear semantics.  Are they meant to flag programming 
errors (in which case they should be replaced with assert) or to log 
warnings about unusual events (in which case SAL_WARN is fine).

Some cases (like the example above) seem to give it away, by explicitly 
handling condition c after DBG_ASSERT'ing !c.  But even that can be an 
outgrowth of unhelpful "defensive programming" rather than competent code.

Of course, it is often anything but trivial to decide the right 
replacement in an existing piece of code.  So maybe we do need to do 
some "light-hearted" mass conversions for the sake of getting rid of the 
old macros after all.  My original hope was that, after replacing all 
occurrences of the old macros, every occurrence of the "good" ones 
(assert, SAL_WARN, SAL_INFO) would be faithful to its semantics.  But 
that is probably already muddied past repair, anyway. ;)

(Not picking on this particular commit, and didn't verify closely 
whether any of the replacements in it are actually "unfaithful".  Just 
thought it couldn't hurt to mention this again.)