Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable meta examples when ENABLE_TESTING is OFF. #3611

Merged
merged 1 commit into from Feb 15, 2017

Conversation

geektoni
Copy link
Contributor

Resolve issue #3553


# Suppress unused variable warnings
# if we are not testing the meta_examples
IF (NOT ENABLE_TESTING)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think I like this

@@ -45,3 +53,10 @@ ENDFOREACH()
add_custom_target(build_cpp_meta_examples ALL
DEPENDS ${GENERATED_CPP_EXAMPLES}
COMMENT "Compiled generated cpp examples")

IF (NOT ENABLE_TESTING)
MESSAGE(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can put that to the place where you set the flags rather than down here?
Also, the indentation is a bit weird

@@ -27,18 +27,25 @@ FOREACH(META_EXAMPLE ${META_EXAMPLES})
LIST(APPEND EXAMPLE_LISTINGS ${CMAKE_CURRENT_BINARY_DIR}/r/${BASENAME}.R)
LIST(APPEND EXAMPLE_LISTINGS ${CMAKE_CURRENT_BINARY_DIR}/lua/${BASENAME}.lua)

# Set generate.py flags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation not respecting the previous one, makes reviewing much harder for me btw

@karlnapf
Copy link
Member

karlnapf commented Feb 9, 2017

This is not the solution that I mentioned in #3553
Can you explain what you are doing here? Not 100% sure I get what you do ... build still passes though so it seems ok

Also, the warning thing with the flag, could you put that into another patch to separate concerns here?

@geektoni
Copy link
Contributor Author

geektoni commented Feb 11, 2017

Hi @karlnapf,
Basically, since the WrappedObjectArray class is guarded and cannot be used when we are not testing the meta examples, I decided to prevent the inclusion of WrappedObjectArray when we are generating the meta examples.

The generator.py take a --store-vars flag that, if used, inserts into the meta example the code to include WrappedObjectArray.

The patch behaviour is:

  • If ENABLE_TESTING is OFF, then we don't use the --store-vars flag and no WrappedObjectArray code is added;
  • If ENABLE_TESTING is ON, then we do use the flag and the meta examples code will have the library;

I thought this was the best approach to solve this problem and I understood this was the way to proceed when I read the issue you made, but probably I didn't catch your idea, my bad :).

Secondly, I wouldn't like to split the unused variable warnings into another patch because we have unused variable warning only when we produce meta example code that has not WrappedObjectArray. If I do that we will have two patches that rely one on the other, and if this is a wrong way to fix the issue the unused variable warning patch will be of little use.

However, if you prefer to have two separate patch I will gladly create another one. :)

By the way, I moved the unused variable warning and its message close together (since it is more logical as you pointed out), but I don't understand why it shows that bad indentation (I am using tab characters tabsize 4 as specified by yours developer guide).

UPDATE 13/02/2014: I had to move the unused variable warning message to its previous location because it was behaving too verbosely (see this: http://pastebin.com/MF5RDjce).

@geektoni geektoni force-pushed the patch-3 branch 2 times, most recently from ec6437f to c850472 Compare February 13, 2017 14:54
@karlnapf
Copy link
Member

Thanks for the good explanation :) I agree with the variable stuff. I should have realised this but I think I looked at this on my phone...

Very nice work! Merging!

@karlnapf karlnapf merged commit 3130c15 into shogun-toolbox:develop Feb 15, 2017
@geektoni geektoni deleted the patch-3 branch February 26, 2017 10:49
karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017
Enable meta examples when ENABLE_TESTING is OFF.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants