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
Check for includes in header files #2557
Comments
@tklein23 could you post a shell magic script for that here? something that output the filenames of header files that include something else than |
hi.. I am working on this .. should i go through each header file or is their some other approach?? |
@srgnuclear, a script that goes through the header files and reports everything included that is not However, I think this is not so straightforward. We have some header files, like our entry point for eigen3 in mathematics which includes Eigen3 headers. Maybe we could start by checking that SWIG is not going through non-Shogun files. I am curious about whether this would make any impact in compilation time/memory. I think it won't. |
@iglesias, k i will look up the script. In the meantime i manually went through header files in src and found some includes apart from < shogun/... > like < vector > ,< stdio.h > ,< glpk.h > ,< cmath > ,< cassert > ,< math.h > ,< limits.h > ,< ctype.h > ,< string.h > ,< stdlib.h > ,< time.h > etc do these also have to be removed because these are some basic includes used frequently. Also if this is not having considerable impact should i continue with it? |
On 31 October 2014 14:29, Saurabh Goyal notifications@github.com wrote:
If possible, try to move these includes from header files to their
Does that make sense to you?
|
yup a lot better now .. i ll work on it and keep you posted .. |
Can we have that one line command here??. Because going through each header file will be very tedious. |
Here you go
produces a lot of hints. Not all of those need to be remove, but most I would say. Oh and very important, please do one PR per class. Dont mix up too many things, we want travis to make sure everything compiles before merging such things. Run all tests locally before sending PRs
|
sure ..!! |
I transfered all non shogun includes from header files to .cpp in classifier. After doing this when i compiled i got errors like - /In file included from /home/saurabh/shogun-install/shogun-3.1.1/src/shogun/../shogun/classifier/mkl/MKLMulticlassGLPK.h:18:0, |
Sure, @srgnuclear. If you remove includes from headers and those headers are relying on the includes to make use of classes or methods defined in those headers, then you are going to get compile errors. What you should check is if the headers really need to have these includes. It can be, for instance, that the dependency comes from the implementation of a method in a header. Then, you can just move the implementation to the cpp file (this does not apply, at least in a straightforward way, for template classes). Another case is that a header is just making use of a class in a method signature (as the return value or argument). Then forward declaration in the header should be enough. This is not an exhaustive explanation covering every case but I hope that it at least you gives a better picture of what's going on. |
You have to do this one by one, not all of them at once. And then compile after each change. |
I went through each class one by one and compiled on making changes and results are i transfered non shogun includes from header files to .cpp for all classes succesfully in classifier except for ./classifier/mkl/MKLMulticlassOptimizationBase.h:#include < vector > which gave compilation error on transfering < vector > include to .cpp. |
The error message should help you solve the problem. What is it exactly? Are there any vectors in the interfaces? |
Few errors like /home/saurabh/shogun-install/shogun-3.1.1/src/shogun/../shogun/classifier/mkl/MKLMulticlassOptimizationBase.h:52:48: error: expected ‘,’ or ‘...’ before ‘<’ token In file included from /home/saurabh/shogun-install/shogun-3.1.1/src/shogun/classifier/mkl/MKLMulticlassGradient.cpp:14:0: |
Try forward declaring vector |
changes in classifiers have been merged in - #2646. |
just grep for includes and change all classes in that folder |
i have changed all classes in classifier folder which folder should i be picking up next? |
I went on with transfer folder where in multitask several header files did not have corresponding .cpp files so for now i have checked only for those which had their .cpp files. |
some of @lisitsyn s stuff is header only I think.... Sergey? |
Includes for other folders removed or shifted to .cpp #2557
all folders except lib and mathematics have been merged. Should i proceed with lib now?? |
yeah go for it :) Ah wait, but lib is somehow different as there might be external libraries in there. Make sure to focus on those classes that are exposed via SWIG first |
i tried to compile the shogun file after building it using cmake. But I am encountering error. The details of compilation is as follows: Edit by @karlnapf : @spandan1234 Sorry for editing your post, but this is both off-topic and please dont paste such long outputs in github threads. Just open a new one and post a gist link with the error message. And read on how to communicate on github, otherwise it is unlikely that you get help. |
What about adding a simple check to cmake that scans all Shogun header files for includes that are not coming from Shogun? This way, we could for example avoid to have Eigen3 includes in headers -- Shogun just wont compile in that case. Would maybe save us some trouble
In fact, this is also an entrace task: remove all non-shogun includes from header files and move to implementation .cpp files
The text was updated successfully, but these errors were encountered: