fnc12 / Mitsoko

10 stars 1 forks source link

ifndef and pragma #3

Closed DonaldTrump88 closed 6 years ago

DonaldTrump88 commented 7 years ago

I started process to build the library.

  1. I saw you used #pragma once. But it is not reliable as #ifndef. Will you prefer to chnage it to ifndef? https://stackoverflow.com/questions/787533/is-pragma-once-a-safe-include-guard https://stackoverflow.com/questions/1143936/pragma-once-vs-include-guards
  2. There are some files Dispatch.hpp, Disposable.hpp, Presenter.hpp, Util.hpp, View.hpp, ViperGod.hpp which have #ifndef. But they are in Titlecase. General programming convention is to use BLOCKCASE.
  3. I saw some places where APPLE and ANDROID used inconsistently. In my opinion
#ifdef __ANDROID__

#elif __APPLE__

#endif

will be better.

  1. Include paths are inconsistent. Which are include paths for the project?
  2. I think, each header file of should have preprocessor error checking as per its group. For android file
    #ifndef __ANDROID__
    #pragma error("Android file without __ANDROID_ preprocessor directive.")
    #endif
  3. Do you think that AndroidUtil.hpp and iOSutil.hpp should include their headers if their preprocessor directive is defined?
fnc12 commented 7 years ago
  1. Yes I thought about #PRAGMA ONCE but it works fine for all target platforms. Of course we can replace it with #ifndef __SOURCE_NAME__ - I'll do it if I have time.

  2. What do you mean 'Titlecase' and 'BLOCKCASE'? Caps? Please provide an example

  3. Yes

    
    #ifdef __ANDROID__

elif APPLE

endif

is better then
```c++
#ifdef __APPLE__

#else

#endif

in case if we add e.g. Windows Phone or any other third platform. But also let's change it. It's more clever to change it - you're right.

  1. Include paths are correct. You need to specify them in your Xcode build settings and Android.mk file. What header can't be found?

  2. You mean in every source in AndroidUtil dir add

    #ifndef __ANDROID__
    #pragma error("Android file without __ANDROID_ preprocessor directive.")
    #endif

    ? If so iOS won't compile and reverse. Android- and iOS- Utils are designed to exist on different platform cause in this case developer can add using android::widget::ListView or using namespace android without #ifdef. It is more comfortable. The goal of mitsoko framework is to make a comfortable tool for building crossplatform apps with C++.

  3. No. It's fine. All classes exist on every platform in runtime but their body is included in #ifdef directives. It allows you to write using java::lang::String without #ifdef

fnc12 commented 7 years ago

It would be great to erase #ifdef for developers cause this feature is ugly.

DonaldTrump88 commented 7 years ago
  1. Titlecase: #ifndef Disposable_h, BLOCK CASE #ifndef DISPOSABLE_H
  2. In template project, I see that you used
    LOCAL_C_INCLUDES := $(LOCAL_PATH)/../../../../../Core/ \
                    $(LOCAL_PATH)/../../../../../Core/libs/ 

    What is the expected include path directory containing ViperGod.hpp?

  3. Let me think more about it.
  4. I meant in AndroidUtil.hpp
    
    #ifdef __ANDROID__
    //all include statements and code. 
    #endif
fnc12 commented 7 years ago
  1. Ok. Let's change it.
  2. ViperGod.hpp is located at Core/Mitsoko. So #include "Mitsoko/ViperGod.hpp" must work.
  3. No. Otherwise you won't be able to write using namespace android without #ifdef __ANDROID__
DonaldTrump88 commented 7 years ago
  1. I feel that ViperGod.hpp is in include folder. Or folder which has ViperGod.hpp should be used as include folder. Avoiding Mitsoko infront of will avoid adding one more level. It should be like "#include "ViperGod.hpp". There are some places where you refered some includes using path relative root path and current path.

    Example: AndroidUtil.hpp
    #include "Mitsoko/Util.hpp" //root path
    #include "java/lang/Array.hpp"  //current path
  2. You are right.

DonaldTrump88 commented 7 years ago

1 and 2 are done. Check pull request.

DonaldTrump88 commented 7 years ago

Added changes for 3. Check pull request.

DonaldTrump88 commented 7 years ago

The remaining point.

  1. I saw some places where APPLE and ANDROID used inconsistently. In my opinion
#ifdef __ANDROID__

#elif __APPLE__

#endif
fnc12 commented 7 years ago

Of course you can fix it - this thing will be required once some third platform appears in Mitsoko framework (e.g. Windows Phone). If you want please fix it but we need to test it first before commiting cause I have working apps that pull mitsoko core on every update

DonaldTrump88 commented 7 years ago

I do not need it. But I can fix it once I get this code running.

fnc12 commented 7 years ago

Ok cool. Please go forward

fnc12 commented 6 years ago

hey. Is the issue still alive?

DonaldTrump88 commented 6 years ago

closed