bugFreeciv - Bugs: bug #20519, Functionality inside...

 
 
Show feedback again

bug #20519: Functionality inside fc_assert_ret() in unit unloading

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Sun 17 Feb 2013 10:02:26 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating System: None
Planned Release: 2.4.0, 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 20 Feb 2013 09:16:09 AM UTC, SVN revision 22394:

Moved necessary functionality out from fc_assert_ret_val()
Functionality of fc_assert_ret_val() currently happens to be included in
release builds too, so this was not causing real problems, but was
just a style issue.

See gna bug #20519

(Browse SVN revision 22394)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 20 Feb 2013 09:16:04 AM UTC, SVN revision 22393:

Moved necessary functionality out from fc_assert_ret_val()
Functionality of fc_assert_ret_val() currently happens to be included in
release builds too, so this was not causing real problems, but was
just a style issue.

See gna bug #20519

(Browse SVN revision 22393)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 17 Feb 2013 10:58:35 AM UTC, comment #4:

I agree with you.

pepeto <pepeto>
Project Member
Sun 17 Feb 2013 10:54:22 AM UTC, comment #3:

> It looks like fc_assert_ret() isn't disabled in NDEBUG builts.


Right. So some macros of the family are affected by NDEBUG and others are not. I can see why it's made that way, but I don't necessarily like the inconsistency. Well, maybe they are fine defined that way, but then we force the patch #3712 CodingStyle policy that no necessary functionality is allowed to be put inside macro calls even in case of those macros for which current implementation allows it. 1) It's easier to remember "Never" than "In case of this and that macro it's ok, but not with those two" 2) things may change in the future.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 17 Feb 2013 10:38:21 AM UTC, comment #2:

It looks like fc_assert_ret() isn't disabled in NDEBUG builts.

pepeto <pepeto>
Project Member
Sun 17 Feb 2013 10:11:32 AM UTC, comment #1:

(Bug #20084 was a previous, very similar instance of this problem.)

Jacob Nevins <jtn>
Project Administrator
Sun 17 Feb 2013 10:02:26 AM UTC, original submission:

Functionality to remove cargo from the list of transporter's transported units is inside fc_assert() so it won't happen at all when asserts are disabled.

Fix attached

I'm worried. The fact that there's some such cases of functionality inside fc_assert() indicates that someone has not been aware of its dangers and could have made same mistake in many places.
I'll see if CodingStyle needs updating in this respect.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 10:10:05 AM UTCjtnDependencies-=>bugs #20722 is dependent
    Wed 20 Feb 2013 11:17:28 PM UTCjtnSummaryUnloading problem in release builds=>Functionality inside fc_assert_ret() in unit unloading
    Wed 20 Feb 2013 09:16:25 AM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun 17 Feb 2013 10:02:26 AM UTCcazfiAttached File-=>Added ReleaseTransportedList.patch, #17234
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup