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

removed using namespace std from pca_imp.hpp #2133

Merged
merged 1 commit into from Jan 4, 2020
Merged

Conversation

@aabghari
Copy link

aabghari commented Dec 30, 2019

No description provided.

@@ -95,11 +93,11 @@ double PCA<DecompositionPolicy>::Apply(arma::mat& data,
// Parameter validation.
if (newDimension == 0)
Log::Fatal << "PCA::Apply(): newDimension (" << newDimension << ") cannot "
<< "be zero!" << endl;
<< "be zero!" << std::endl;

This comment has been minimized.

Copy link
@zoq

zoq Dec 30, 2019

Member

Since this isn't part of a header file, I don't necessary see the benefit of using the std:: prefix. I guess sometimes it makes it more clear, but I think for endl it's pretty clear. Anyway, I'm not against merging this in, but if we do we should update the style guidelines and use the same style over the codebase.

This comment has been minimized.

Copy link
@aabghari

aabghari Dec 30, 2019

Author

If I am not mistaken, pca.hpp includes pca_imp.hpp

This comment has been minimized.

Copy link
@zoq

zoq Jan 1, 2020

Member

Right, what I meant is a user wouldn't include the pca_impl.hpp.

This comment has been minimized.

Copy link
@aabghari

aabghari Jan 1, 2020

Author

to be clear, the problem is not the style. The issue is by including pca.hpp in your code, the std namespace is getting used by default. It pollutes the application namespace.

This comment has been minimized.

Copy link
@zoq

zoq Jan 2, 2020

Member

Maybe I missed something but you modified pca_impl.hpp not pca.hpp; pca_impl.hpp is the implementation, pca.hpp is the part that is going to be included by an application.

I'm certainly not against the change, but if we do that, let's do it for all the other places as well.

This comment has been minimized.

Copy link
@zoq

zoq Jan 2, 2020

Member

So to pollute the application namespace, and we talk here about some application that links against mlpack, not mlpack itself. The application would include pca.hpp but that doesn't mean it can use anything from std because pca_impl.hpp uses using namespace std;. Maybe I still misunderstood your point.

This comment has been minimized.

Copy link
@aabghari

aabghari Jan 2, 2020

Author

The following example compiles without any error when pca.hpp is included:

#include <cstdlib>
#include <mlpack/methods/pca/pca.hpp>
#include <string>
#include <iostream>

/*
 * 
 */
int main(int argc, char** argv) {
    
    string str = "Hello world";
    cout << str << endl;
    return 0;
}

Whereas the same code without pca.hpp included gives the following errors:

main.cpp:11:5: error: ‘string’ was not declared in this scope
main.cpp:12:5: error: ‘cout’ was not declared in this scope
main.cpp:12:13: error: ‘str’ was not declared in this scope
main.cpp:12:20: error: ‘endl’ was not declared in this scope

As you can see, by including the pca.hpp into your code, the std namespace is used by default which is completely wrong.

This comment has been minimized.

Copy link
@zoq

zoq Jan 2, 2020

Member

Thanks for being persistent, if this works, we should definitely fix this, do you mind to fix the other places as well? I can also run a regex over the codebase and change the necessary places, up to you.

This comment has been minimized.

Copy link
@aabghari

aabghari Jan 2, 2020

Author

I checked the source code and I think that's the only place, but it doesn't hurt if you double check too.

This comment has been minimized.

Copy link
@zoq

zoq Jan 2, 2020

Member

Looks like you are right.

@zoq
zoq approved these changes Jan 2, 2020
Copy link
Member

zoq left a comment

Thanks again for being persistent, looks good to me.

@mlpack-bot
mlpack-bot bot approved these changes Jan 4, 2020
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@zoq zoq merged commit 16ee2fa into mlpack:master Jan 4, 2020
18 checks passed
18 checks passed
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
mlpack.mlpack Build #20191230.8 succeeded
Details
mlpack.mlpack (Linux Julia) Linux Julia succeeded
Details
mlpack.mlpack (Linux Markdown) Linux Markdown succeeded
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Linux Python27) Linux Python27 succeeded
Details
mlpack.mlpack (Linux Python37) Linux Python37 succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Julia) macOS Julia succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
@zoq

This comment has been minimized.

Copy link
Member

zoq commented Jan 4, 2020

Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.