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

Include a third-party JSON library #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Include a third-party JSON library #2

wants to merge 5 commits into from

Conversation

ylee88
Copy link
Collaborator

@ylee88 ylee88 commented Jan 23, 2025

nlohmann/json is a header-only library, so it can be included in this repo as a third party. This PR will let the user build Milhoja without cloning nlohmann/json by themself.

@ylee88 ylee88 requested a review from kweide January 23, 2025 22:57
Copy link
Member

@kweide kweide left a comment

Choose a reason for hiding this comment

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

Using this file json.hpp is INSANE overhead... ~ 920,000 bytes in 24765 source lines, just to write one JSON file... Maybe I am missing something, but the only real use of this header file appears to be in tools/createSizesJson.cpp, for writing a few lines to the file sizes.json. (That and some use for testing, see test/RuntimeParameters.h).

Moreover, the file tools/createSizesJson.cpp already has code for writing sizes.json, it is just commented out! See

        //output << "{" << std::endl;                                                                                                                       

        // TODO: There should be a better way to print the name of a data                                                                                   
        //       type and its size by using a std::tuple object as well as                                                                                  
        //       typeid(T).name(). This way we can create a tuple object filled                                                                             
        //       with types, making it easy to loop over as well as add new                                                                                 
        //       types if necessary. Also, GCC would not need a special env                                                                                 
        //       variable for using a specific json library. However, for the                                                                               
        //       sake of getting the datapacket_tests into main, I'll ignore                                                                                
        //       this for now.                                                                                                                              
        //output << "    \"byte_align\": " << BYTE_ALIGNMENT << "," << std::endl;                                                                           
        //output << "    \"int\": " << sizeof(int) << "," << std::endl;                                                                                     
        //output << "    \"unsigned int\": " << sizeof(unsigned int) << "," << std::endl;                                                                   
        //output << "    \"std::size_t\": " << sizeof(std::size_t) << "," << std::endl;                                                                     
        //output << "    \"real\": " << sizeof(milhoja::Real) << "," << std::endl;                                                                          
        //output << "    \"IntVect\": " << sizeof(milhoja::IntVect) << "," << std::endl;                                                                    
	//output << "    \"RealVect\": " << sizeof(milhoja::RealVect) << "," << std::endl;                                                                  
	//output << "    \"FArray1D\": " << sizeof(milhoja::FArray1D) << "," << std::endl;                                                                  
	//output << "    \"FArray2D\": " << sizeof(milhoja::FArray2D) << "," << std::endl;                                                                  
	//output << "    \"FArray3D\": " << sizeof(milhoja::FArray3D) << "," << std::endl;                                                                  
	//output << "    \"FArray4D\": " << sizeof(milhoja::FArray4D) << std::endl;                                                                         
	output << sizes.dump(INDENT);

	//output << "}" << std::endl;                                                                                                                       

All it should take is uncommenting the commented code lines, and vice versa, in the above code snippet - and removing the include. (Untested)

I am willing to create an alternative PR tomorrow to get rid of this dependency on json.hpp.

@ylee88
Copy link
Collaborator Author

ylee88 commented Jan 24, 2025

I don't think it is a big overhead; as you already pointed out, the only file has #include <nlohmann/json.hpp> line is just tools/createSizesJson.cpp. So this giant header file is only included in compiling tools/createSizesJson.cpp.

But I agree that using an external library just to write a simple JSON file may be an overkill. I think Jaread wanted to make this more extendable by using a well-known library. I prefer to use an existing library instead of writing some logic ourselves.

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.

2 participants