[MOAB-dev] r4016 - MOAB/trunk/tools/dagmc

[Paul Wilson]

Alas, I find myself more convinced of the others' view.  

It seems dangerous for the public interface to depend on a define, thus 
allowing the library to be built with one interface and the calling app 
to be built expecting another.  (Perhaps in an earlier email to me) 
Steve pointed out that moab.make is more of a convenience than a 
requirement for building apps with MOAB.  We could include relevant 
defines in moab.make (one way or another) but it still allows for the 
possibility that this inconsistency arises when a developer decides not 
to use moab.make. 

Furthermore, it was a particularly difficult inconsistency to track down 
as there were no compile-, link-, or runtime errors - just the wrong 
answer when the code ran!

Note, also, that CGM defines are still available after this fix, they 
just don't change the public interface.

Some other ideas:

    * Modify the header (eg. DagMC.hpp) at install-time to install a
      header that is unconditionally the same as the conditions used in
      building MOAB.  That is, parse the header at install time with the
      same macros as used to build so that the interface is consistent
      and unambiguous
    * <begin kludge (I'm brainstorming here)>Declare a function
      prototype and/or invoke a function within the conditional code
      that will at least raise a link-time error when the inconsistency
      arises</kludge>

Paul



Tim Tautges wrote:
> I agree with Paul, if MOAB is compiled with CGM on, the CGM defines 
> etc. should be available to client apps, probably via inclusion of 
> cgm.make in moab.make.
>
> Jim, could you investigate?  Thanks.
>
> - tim
>
> On 06/23/2010 04:19 PM, Paul Wilson wrote:
>> Hi Steve,
>>
>> Thanks again for finding this, but the more I think about it, I wonder
>> if this is the right solutions.
>>
>> It is theoretically possible to build MOAB/DAGMC apps with no reference
>> to CGM whatsoever. Under those circumstances, RefEntity will never be
>> defined beyond the single declaration in DagMC.hpp. Is this OK? Wouldn't
>> it be better to set the CGM macro in moab.make for inclusion in other 
>> apps?
>>
>> Paul
>>
>> sjackson at cae.wisc.edu wrote:
>>> Author: sjackson
>>> Date: 2010-06-23 14:44:46 -0500 (Wed, 23 Jun 2010)
>>> New Revision: 4016
>>>
>>> Modified:
>>> MOAB/trunk/tools/dagmc/DagMC.hpp
>>> Log:
>>> Fix a very old, but recently symptomatic, bug in DagMC.
>>>
>>> When MOAB is built with CGM, the macro "CGM" is defined. Until now,
>>> this caused an extra data member to be included in the DagMC class.
>>> However, client code compiling against DagMC does not define the CGM
>>> macro (the macro is not exported to moab.make). This means a client
>>> compiling against DagMC.hpp can wind up with an incorrect map of the
>>> data members in a DagMC object.
>>>
>>> This hasn't surfaced as a problem before because the conditionally-
>>> included data member was declared *after* all other client-facing
>>> data members. The reordering of data members in r4006 exposed the
>>> problem.
>>>
>>> The easy solution (which I've taken) is to simply remove the checks
>>> from the header file. If CGM is not available, the extra declarations
>>> are harmless (and DagMC gets one extra unused std::vector).
>>>
>>> Moral: Beware of #ifdefs in public headers, particularly if they
>>> check for macros that client code may not know about.
>>>
>>> Modified: MOAB/trunk/tools/dagmc/DagMC.hpp
>>> ===================================================================
>>> --- MOAB/trunk/tools/dagmc/DagMC.hpp 2010-06-22 18:15:50 UTC (rev 4015)
>>> +++ MOAB/trunk/tools/dagmc/DagMC.hpp 2010-06-23 19:44:46 UTC (rev 4016)
>>> @@ -12,9 +12,7 @@
>>>
>>> #include "moab/OrientedBoxTreeTool.hpp"
>>>
>>> -#ifdef CGM
>>> class RefEntity;
>>> -#endif
>>>
>>> namespace moab {
>>>
>>> @@ -349,10 +347,8 @@
>>> // entity index (contiguous 1-N indices) indexed like rootSets are
>>> std::vector<int> entIndices;
>>>
>>> -#ifdef CGM
>>> // corresponding geometric entities indexed like rootSets are
>>> std::vector<RefEntity *> geomEntities;
>>> -#endif
>>> // metadata
>>> Tag matTag, densTag, bcTag, impTag, tallyTag;
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>

-- 
Paul Wilson

-- ------------------------------------------------------------------ --
Paul P.H. Wilson              419 Engineering Research Building
wilsonp at engr.wisc.edu                       1500 Engineering Dr
ph/fax: 608/263-0807                          Madison, WI 53706

           My calendar: http://bit.ly/pphw-calendar

       Computational Nuclear Engineering Research Group
               http://cnerg.engr.wisc.edu

          Associate Professor, Nuclear Engineering
              Engineering Physics Department     
               http://www.engr.wisc.edu/ep       

        Chair, Energy Analysis & Policy Certificate
         Nelson Institute for Environmental Studies
                http://nelson.wisc.edu/eap/

          Contributing to the joy and improvement
                  of all those around me

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/moab-dev/attachments/20100624/bdebec59/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3297 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.mcs.anl.gov/pipermail/moab-dev/attachments/20100624/bdebec59/attachment-0001.bin>