Issue 5326829724368896: 1521 - libadblockplus is not compilable on linux x64 (Closed)
|
Created:
Jan. 13, 2015, 3:24 p.m. by paco Modified:
June 15, 2015, 11:09 a.m. CC:
mathias, aalvz, sergei Visibility:
Public. |
DescriptionSince we are not updating the v8 code, we need to ensure that only warnings for that code are ommited, with this change, we will ensure that
Patch Set 1 #
Total comments: 4
Patch Set 2 : '1521 - libadblockplus is not compilable on linux x64' #
Total comments: 6
Patch Set 3 : 1521 - libadblockplus is not compilable on linux x64 #MessagesTotal messages: 14
If this actually fixes the issue, I really don't understand why :D http://codereview.adblockplus.org/5326829724368896/diff/5629499534213120/liba... File libadblockplus.gyp (right): http://codereview.adblockplus.org/5326829724368896/diff/5629499534213120/liba... libadblockplus.gyp:22: 'direct_dependent_settings':{ There already is a direct_dependent_settings section below - why not use that? Will having two even work as intended? http://codereview.adblockplus.org/5326829724368896/diff/5629499534213120/liba... libadblockplus.gyp:24: 'werror', What exactly is this supposed to do? How I understand it, it just adds a preprocessor definition called "werror". Why? I can't find anything in the v8 code looking for that. http://codereview.adblockplus.org/5326829724368896/diff/5629499534213120/liba... libadblockplus.gyp:27: 'third_party/v8/include', This adds the v8 include path to all projects using the library - something we explicitly avoided. Why? http://codereview.adblockplus.org/5326829724368896/diff/5629499534213120/liba... libadblockplus.gyp:31: 'include', Let's stick to two space indents pleasee.
For the record: This seems roughly like what we'll want to do here: https://code.google.com/p/gyp/wiki/GypUserDocumentation#Exclude_settings_on_a...
http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (left): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:15: 'cflags': [ '-Wall', '-W', '-Wno-unused-parameter', Since we're using `-Wall` the other flags are not necessary http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (right): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:18: 'cflags!': [ '-Werror', ], We're gonna see the errors but we're not treating them as erros
http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (right): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:16: '-std=c++0x', '-fexceptions', ], previously there were spaces.
http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (left): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:15: 'cflags': [ '-Wall', '-W', '-Wno-unused-parameter', On 2015/01/14 15:15:58, paco wrote: > Since we're using `-Wall` the other flags are not necessary -W is not implied by -Wall (see http://gcc.gnu.org/onlinedocs/gcc-3.0/gcc_3.html). Also, -Wno-unused-parameter *suppresses* a warning. However, -Wnon-virtual-dtor seems unnecessary indeed.
> *suppresses* a warning. However, -Wnon-virtual-dtor seems unnecessary indeed. The intention is not supress the warnings but let them through but no taking them as errors
Sorry for the delay! http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (left): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:15: 'cflags': [ '-Wall', '-W', '-Wno-unused-parameter', On 2015/01/22 15:29:21, Wladimir Palant wrote: > On 2015/01/14 15:15:58, paco wrote: > > Since we're using `-Wall` the other flags are not necessary > > -W is not implied by -Wall (see > http://gcc.gnu.org/onlinedocs/gcc-3.0/gcc_3.html). Also, -Wno-unused-parameter > *suppresses* a warning. However, -Wnon-virtual-dtor seems unnecessary indeed. Are these changes really necessary to fix the build on 64-bit Linux? If not, let's move them to a different review please (can be Noissue if it's just about removing obsolete flags). http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... File common.gypi (right): http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... common.gypi:18: 'cflags!': [ '-Werror', ], On 2015/01/14 15:15:58, paco wrote: > We're gonna see the errors but we're not treating them as erros I think it'd make sense to move this right below cflags - but it looks like the proper solution indeed.
On 2015/02/02 14:12:37, Felix H. Dahlke wrote: > Sorry for the delay! > > http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... > File common.gypi (left): > > http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... > common.gypi:15: 'cflags': [ '-Wall', '-W', '-Wno-unused-parameter', > On 2015/01/22 15:29:21, Wladimir Palant wrote: > > On 2015/01/14 15:15:58, paco wrote: > > > Since we're using `-Wall` the other flags are not necessary > > > > -W is not implied by -Wall (see > > http://gcc.gnu.org/onlinedocs/gcc-3.0/gcc_3.html). Also, -Wno-unused-parameter > > *suppresses* a warning. However, -Wnon-virtual-dtor seems unnecessary indeed. > > Are these changes really necessary to fix the build on 64-bit Linux? If not, > let's move them to a different review please (can be Noissue if it's just about > removing obsolete flags). > > http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... > File common.gypi (right): > > http://codereview.adblockplus.org/5326829724368896/diff/5685265389584384/comm... > common.gypi:18: 'cflags!': [ '-Werror', ], > On 2015/01/14 15:15:58, paco wrote: > > We're gonna see the errors but we're not treating them as erros > > I think it'd make sense to move this right below cflags - but it looks like the > proper solution indeed. BTW, I meant, we're gonna see the warning but not treating them as errors
LGTM
LGTM
Pushed now: https://hg.adblockplus.org/libadblockplus/rev/90e8271246c1 Sorry for the delay, didn't realise I have to push it before you asked, Paco. Please close review and issue. 
|
