Mir

Code review comment for lp:~hikiko/mir/mir.dest-tmp

Revision history for this message
Eleni Maria Stea (hikiko) wrote :

@Daniel: thanks

> 1. I think we went off topic when noexcept was added to:
> + virtual ~MockAndroidRegistrar() {}
> + virtual ~MockANativeWindow() {}
> + virtual ~MockImplementation() {}
> That really should be a separate proposal as this one was meant to be just
> about fixing virtual destructors... ?

If you don't add the noexcept and you use = default in the parent you get a compile error in gcc because of these:
http://stackoverflow.com/questions/11497252/default-destructor-nothrow
https://code.launchpad.net/~hikiko/mir/mir.fix-virt-destructorS/+merge/167705/comments/372088
https://code.launchpad.net/~hikiko/mir/mir.fix-missing-virtual-destructors/+merge/165464/comments/368890

>
> 2. This change is still unnecessary as it's already included in -Wall:
> 60 -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -std=c++0x -Werror -Wall
> -fno-strict-aliasing -pedantic -Wextra -fPIC")
> 61 +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -std=c++0x -Werror -Wall
> -fno-strict-aliasing -pedantic -Wnon-virtual-dtor -Wextra -fPIC")
> Making the global CMAKE_CXX_FLAGS unnecessarily more complex is something to
> avoid.
>
ok, you corrected yourself (https://code.launchpad.net/~hikiko/mir/mir.dest-tmp/+merge/168934/comments/379703)

> 3. Various blank lines have been added and removed. And in some cases they're
> the only change made to a file, so should be undone.

I just leave a new line after the constructors-destructors "block" of lines when there are other functions that follow... The only empty line introduced in an empty file is 144 and is there after merge to trunk/resolve (sorry!). But anyway I'll remove all the empty lines...

« Back to merge proposal