search

redserpent7 / cld2

Automatically exported from code.google.com/p/cld2
0 stars 0 forks source link

Consider declaring dynamic data methods unconditionally #16

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Today, we guard the declaration of the dynamic-data-related functions in 
comapct_lang_det.h with "#ifdef CLD2_DYNAMIC_MODE":
https://code.google.com/p/cld2/source/browse/trunk/public/compact_lang_det.h

This causes some unfortunate side effects when including CLD2 in another 
project: unless building with a single compile pass including all sources, any 
separate compilation unit that requires dynamic functionality has to have the 
same define when it #includes compact_lang_det.h in order to keep the compiler 
happy.

For example, Chromium builds CLD2 separately, then links it into the Chromium 
binary; but if CLD2_DYNAMIC_MODE isn't defined in Chromium code that includes 
compact_lang_det.h, you get compiler errors like the ones below even if CLD2 
itself has been built with the define:

error: 'isDataLoaded' is not a member of 'CLD2'
error: 'loadDataFromRawAddress' is not a member of 'CLD2'

Ideally, the #define guard can be encapsulated entirely within CLD2 so that the 
dependent library doesn't need to know about this at all.

The downside is that dependent code might accidentally try to use dynamic mode 
even if it isn't available. Throwing exceptions isn't a viable solution, since 
some projects disable exceptions when compiling. We'd presumably just have to 
define the following behavior if CLD2_DYNAMIC_MODE is not defined:

isDataLoaded: return true
loadDataFromRawAddress: no-op and output a warning to stderr
loadDataFromFile: no-op and output a warning to stderr

This change should be fully backwards compatible, since it doesn't change or 
remove any existing function declarations under any circumstances.

Original issue reported on code.google.com by andrewha...@google.com on 23 Jun 2014 at 9:57

GoogleCodeExporter commented 9 years ago 
+toyoshim@chromium.org who raised this issue in Chromium code review 333603002:

https://codereview.chromium.org/333603002

Original comment by andrewha...@google.com on 23 Jun 2014 at 9:58

GoogleCodeExporter commented 9 years ago 
This is looking more important now that Chromium is running into issues that 
exacerbate the problem:
https://code.google.com/p/chromium/issues/detail?id=367239

In a complex project that uses CLD2, having to fork the build logic on 
CLD2_DYNAMIC_MODE is sub-optimal.

I suggest we go ahead with the fix proposed above, with one caveat: While we're 
here, we add a new method:

  bool isDataDynamic()

This method can be declared in the .h and implemented with a simple 
preprocessor check in the c++ code to return either true (if CLD2_DYNAMIC_MODE) 
or false (otherwise). This will still allow dependent projects to be smart. The 
cost is that we will need to bring in a total of 5 function signatures into the 
header (the 4 that already exist, plus the new one); this shouldn't be a big 
deal, though, since there's no extra includes and build optimizations should be 
able to simply discard the unused symbols in most projects. Very low cost, and 
makes life easier. I'm going to go ahead with this.

Original comment by andrewha...@google.com on 25 Jul 2014 at 8:07

GoogleCodeExporter commented 9 years ago 
Committed in r162.

Original comment by andrewha...@google.com on 25 Jul 2014 at 9:56