HDFS-16174. Refactor TempFile and TempDir in libhdfs++ #3303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Need to implement TempFile and TempDir in cc files for avoiding duplicate compilation of these classes.
* Implemented TempDir class in temp-dir.cc file.
* Creating an object library for test utils to help with modularization.
* Wrapped TempDir and TempFile in a TestUtils namespace.
* Added documentation for the TempDir class.
* Removed template type for it to be deduced automatically.
* Removed sys/stat.h from temp-dir.h as it was redundant.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 16, 2021
| @@ -114,105 +111,6 @@ void writeDamagedConfig(const std::string& filename, Args... args) { | |||
| out.open(filename); | |||
| out << stream.rdbuf(); | |||
| } | |||
|
|
|||
| // TempDir: is deleted on destruction | |||
| class TempFile { | |||
There was a problem hiding this comment.
Other than reducing the size of this file, is there any benefit of having this? Or this is just a refactor at this point?
There was a problem hiding this comment.
As I've mentioned in the corresponding JIRA - https://issues.apache.org/jira/browse/HDFS-16174, header files should contain only the declaration of the class and method signatures. The implementation must be done in the cc files. Otherwise, these classes get recompiled everytime they're #included, leading to longer compilation times. Also, it's not a good C++ practice to combine declaration and implementation in the header file, unless it's a template class or method.
Another thing that I've done in this PR is to make this into a test utility library, so that it's more modular and can be imported anywhere in libhdfs++.
There was a problem hiding this comment.
Those are valuable but let's make this a refactor PR to make it more explicit.
GauthamBanasandra
changed the title
goiri
approved these changes
Aug 16, 2021
| @@ -114,105 +111,6 @@ void writeDamagedConfig(const std::string& filename, Args... args) { | |||
| out.open(filename); | |||
| out << stream.rdbuf(); | |||
| } | |||
|
|
|||
| // TempDir: is deleted on destruction | |||
| class TempFile { | |||
There was a problem hiding this comment.
Those are valuable but let's make this a refactor PR to make it more explicit.
GauthamBanasandra
changed the title
goiri
approved these changes
Aug 17, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TempDir in cc files for avoiding
duplicate compilation of these
classes.
TestUtils namespace.
TestUtils and added this as a
target as necessary.