Bug 1793

Summary: Various errors by MacOS compiler
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: lteAssignee: Nicola Baldo <nicola>
Status: RESOLVED FIXED    
Severity: blocker CC: ns-bugs
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: Mac OS   
Attachments: bug fixes
Bug fixes (minimal)

Description Tommaso Pecorella 2013-11-08 14:37:29 UTC
Created attachment 1701 [details]
bug fixes

Even though I don't have clang (yet) on this Mac...
1) NS_ASSERT should be used consistently. E.g., if there is already a check that a variable is between two values, NS_ASSERT shouldn't be used to check that value/2 is in the (half) range.
2) isnan is a std:: function
3) do not check that an uint is greater than 0... it is for sure.

As usual, MacOS is quite sensible to this sort of stuff.

Attached the patch fixing the errors.

By the way, I don't have clang here but the latest patch did include some defined-but-unused member variables in the LTE examples, so more errors on clang.
Comment 1 Nicola Baldo 2013-11-14 13:15:51 UTC
Tommaso, can you please clarify which issues give compilation problems with Mac and which not? I understand 2) and 3), but I do not agree with 1) - if you don't like assertions, you can always compile with -d optimized ;-)
Comment 2 Tommaso Pecorella 2013-11-14 13:54:36 UTC
(In reply to Nicola Baldo from comment #1)
> Tommaso, can you please clarify which issues give compilation problems with
> Mac and which not? I understand 2) and 3), but I do not agree with 1) - if
> you don't like assertions, you can always compile with -d optimized ;-)

Thanks, but I like my code in debug state... since I'm mostly fighting bugs (a lost battle, if you want my opinion).

However you're right, I was a bit too harsh with the asserts.

Here it is a 2nd patch, only covering what's strictly necessary. Asserts checking if an unsigned int is greater than 0 are removed.

On the other hand, I'm still puzzled by code like this:

int8_t
EutranMeasurementMapping::ActualA3Offset2IeValue (double a3OffsetDb)
{
  if ((a3OffsetDb < -15.0) || (a3OffsetDb > 15.0))
    {
      NS_FATAL_ERROR ("The value " << a3OffsetDb
                                   << " is out of the allowed range (-15..15) dB"
                                   << " for A3 Offset");
    }

  uint8_t ieValue = lround (a3OffsetDb * 2.0);
  NS_ASSERT (ieValue <= 30);
  return ieValue;
}

Since a3OffsetDb is less than 15.0, and ieValue is a3OffsetDb*2, ieValue is less than 30 for sure. Unless there something horrible with lround.

In my opinion those type of tests are superfluous.

Cheers,

T.
Comment 3 Tommaso Pecorella 2013-11-14 13:55:40 UTC
Created attachment 1710 [details]
Bug fixes (minimal)
Comment 4 Nicola Baldo 2013-11-15 05:36:19 UTC
It turns out that, while we were wasting time in philosophical discussions, Peter just went ahead and fixed it in changeset f206fb584abf :-) 

Anyway, thanks Tommaso for the updated patch. I integrated it with Peter's change (1 line killed). As for EutranMeasurementMapping::ActualA3Offset2IeValue(), it was actually a bug, a int8_t was to be used instead of an uint8_t. You see that even superfluous assertions turn out to be valuable....


changeset:   10413:3ea02a8923ff
tag:         tip
user:        Nicola Baldo <nbaldo@cttc.es>
date:        Fri Nov 15 11:20:38 2013 +0100
summary:     fixed Bug 1793 - Various errors by MacOS compiler