aliens

Author Topic: OpenXcom and OXCE Plus: logger.h conflicts with PBRT log.h (Log macro problem)  (Read 2220 times)

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
The following two header files are causing a build/compile conflict:

OpenXcom Extended Plus and vanilla OpenXcom engine/logger.h:
https://github.com/MeridianOXC/OpenXcom/blob/oxce-plus/src/Engine/Logger.h

PBRT/util/log.h:
https://github.com/mmp/pbrt-v4/blob/master/src/pbrt/util/log.h

OpenXcom declares/defines a Log macro as follows:

#define Log(level) if (level > Logger::reportingLevel()) { } else Logger().get(level)

However it seems this "macro" is not "confined" to the OpenXcom namespace so it is visible outside of it's name space which is leading to a conflict inside PBRT/util/log.h:

Where a function with the name Log is declared inside a namespace called pbrt
namespace pbrt {

// Logging Function Declarations
PBRT_CPU_GPU
void Log(LogLevel level, const char *file, int line, const char *s);

}

So PBRT is following the official C/C++ language standard. While Macros are not officially part of the C++ language.

Also somebody on stack overflow writes: "No, the preprocessor doesn't care about namespaces at all. ":
https://stackoverflow.com/questions/11791187/is-it-possible-to-place-a-macro-in-a-namespace-in-c

So I blame this conflict on OpenXcom source code base and therefore I am requesting that this gets fixed to prevent conflicts with PBRT and possibly any other library that defines it's own log funtion.

I think the easiest fix would be to replace this define/macro with a function called: Log.

(What would be the side effects ? Maybe slower code because of function being called ?)

This seems to work:

// Skybuck: macro disabled, macros cannot be part of namespaces, this is causing conflict with PBRT/util/log.h Log function inside pbrt namespace.

// #define Log(level) if (level > Logger::reportingLevel()) { } else Logger().get(level)

std::ostringstream& Log( SeverityLevel level )
{
   if (level > Logger::reportingLevel())
   {

   }
   else
   {
      return Logger().get(level);
   }
}

Testing it, gives following warning message:

1>    E:\SourceCode\OpenXComExtended\PBRT\src\Engine\Logger.h(83): warning C4715: 'OpenXcom::Log': not all control paths return a value


*** New Version, which does return an output stream but disables it if it does not match the level requirements, enables it if it does match the level requirements ***:

std::ostringstream& Log( SeverityLevel level )
{
   std::ostringstream& vOutputStream = Logger().get(level);

   if (level > Logger::reportingLevel())
   {
      vOutputStream.setstate( std::ostringstream::failbit );
   }
   else
   {
      vOutputStream.clear();
   }

   return vOutputStream;
}

Testing it now...

Produces errors, which can be surpressed with /FORCE:MULTIPLE

1>Target Link:
1>  BasescapeState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  BaseView.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  BuildFacilitiesState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftArmorState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftEquipmentLoadState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftEquipmentSaveState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftEquipmentState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftInfoState.obj : error LNK2005: "class std::basic_ostringstream<char,struct std::char_traits<char>,class std::allocator<char> > & __cdecl OpenXcom::Log(enum OpenXcom::SeverityLevel)" (?Log@OpenXcom@@YAAAV?$basic_ostringstream@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@W4SeverityLevel@1@@Z) already defined in BaseInfoState.obj
1>  CraftPilotSe

If surpressed still produces many warnings.

Any idea how to fix this better ?

*** New Fix, replace Log with XComLog ***:

Though some Log funtion was replaced, which ig logarithm or something in battlescape, macros are risky ?! (No vs thinks it's Log from PBRT hmm..
 hopefully just intellisense problem)

1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2059: syntax error: 'if'
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2143: syntax error: missing ';' before '{'
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2143: syntax error: missing ')' before ';'
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2181: illegal else without matching if
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2664: 'std::ostringstream &OpenXcom::Logger::get(OpenXcom::SeverityLevel)': cannot convert argument 1 from 'double' to 'OpenXcom::SeverityLevel'
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): message : This conversion requires an explicit cast (static_cast, C-style cast or function-style cast)
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Engine\Logger.h(53,22): message : see declaration of 'OpenXcom::Logger::get' (compiling source file Battlescape\BattlescapeState.cpp)
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,17): error C2059: syntax error: ')'
1>  E:\SourceCode\OpenXComExtended\PBRT\src\Battlescape\BattlescapeState.cpp(3057,35): error C2059: syntax error: ')'
« Last Edit: May 22, 2022, 10:42:50 pm by Skybuck »

Offline Yankes

  • Commander
  • *****
  • Posts: 3194
    • View Profile
macros ARE part of C++ and will never be removed.

Simply do not include both in same cpp file.

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
Not possible,  to not include both... anyway Log replaced by XComLog.

Offline Yankes

  • Commander
  • *****
  • Posts: 3194
    • View Profile
How it could be not possible? You added this new header and this mean you can remove it.

Beside I do not see sense in trying debug your problems when you butcher whole code base to fix small name conflict that could be some times fixed by reordering includes.

Offline Skybuck

  • Colonel
  • ****
  • Posts: 223
    • View Profile
PBRT init requires options, these option structure comes from Options.h. Options.h includes log.h.

Ultimately C/C++ is one big file, all the includes lead to one big file. So any namespace conflict will ultimately show up. The macro cannot be part of any namespace so it can conflict with anything. There is also a second possibility of conflict. Log vs log. log with small letter is logarithm. However C/C++ treats big/small letters seperately.

XComLog looks better than just Log so I like the change and it will be necessary to avoid namespace conflict with any other code base that also has some kind of Log declaration.
(Perhaps you saw butchering in code base in the vide, this was undo later and instead a replace all done :))
« Last Edit: May 24, 2022, 06:56:28 pm by Skybuck »