HPCC-11841 Add utility method getSortedDirectoryIterator() #10602
Conversation
|
https://track.hpccsystems.com/browse/HPCC-11841 |
|
@jakesmith please review. |
common/remote/hooks/git/gitfile.cpp
Outdated
| @@ -227,6 +227,13 @@ class GitRepositoryFile : implements IFile, public CInterface | |||
| return createGitRepositoryDirectoryIterator(dirName, mask, sub, includeDirs); | |||
| } | |||
| } | |||
| IDirectoryIterator *directoryFilesSorted(SortDirectoryMode mode, bool rev, const char *mask=NULL, bool sub=false, bool includedirs=false) | |||
There was a problem hiding this comment.
Missing 'virtual'
and don't need default param. definitions (which are in virtual decl. in header)
There was a problem hiding this comment.
Should use 'override' consistently too really. They prevent mishaps, like getting the override prototype wrong.
system/jlib/jfile.cpp
Outdated
| if (SD_nosort == mode) | ||
| return baseIter; | ||
| return new CSortedDirectoryIterator(*baseIter, mode, rev, includedirs); | ||
| } |
There was a problem hiding this comment.
Am wondering since this is a universal implementation, if it wouldn't be better to implement this in a an abstract CFileBase class.
i.e. the git, archive, sockfile impls. can inherit from that rather than re-implement it.
There was a problem hiding this comment.
Ok, I will implement this in new CFileBase. Since the new directoryFilesSorted() will be removed from the git, archive, etc., I think it would be clear if I rebase the changes into 1 commit for you to review.
| @@ -385,6 +385,13 @@ class ArchiveFile : implements IFile, public CInterface | |||
| return createArchiveDirectoryIterator(dirName, mask, sub, includeDirs); | |||
| } | |||
| } | |||
| IDirectoryIterator *directoryFilesSorted(SortDirectoryMode mode, bool rev, const char *mask=NULL, bool sub=false, bool includedirs=false) | |||
There was a problem hiding this comment.
As above, should be consistent with 'virtual' and don't need default params.
And would benefit from 'override', but should add to other methods to be consistent.
system/jlib/jfile.hpp
Outdated
| @@ -632,4 +633,67 @@ const static bool filenamesAreCaseSensitive = false; | |||
| const static bool filenamesAreCaseSensitive = true; | |||
| #endif | |||
|
|
|||
| class jlib_decl CSortedDirectoryIterator : implements IDirectoryIterator, public CInterface | |||
There was a problem hiding this comment.
Better to use CSimpleInterface unless you need beforeDispose
And better to use templaces to avoid double inheritance.
so:
public CSimpleInterfaceOf
system/jlib/jfile.hpp
Outdated
| sortedFileCount = sortedFiles.length(); | ||
| } | ||
|
|
||
| IMPLEMENT_IINTERFACE |
There was a problem hiding this comment.
Unnecessary with above change.
system/jlib/jfile.hpp
Outdated
|
|
||
| IMPLEMENT_IINTERFACE | ||
|
|
||
| virtual bool first() |
There was a problem hiding this comment.
good to use 'override' here and other overloads.
system/jlib/jfile.hpp
Outdated
|
|
||
| virtual __int64 getFileSize() | ||
| { | ||
| return curDirectoryEntry ? curDirectoryEntry->size : 0; |
There was a problem hiding this comment.
I think should return -1 if invalid (see other implementations)
|
@wangkx - a few comments. |
|
@jakesmith please review. |
wangkx
changed the title
wangkx
changed the title
|
@jakesmith the CFileBase is removed and the getSortedDirectoryIterator() is added. Please review. |
system/jlib/jfile.hpp
Outdated
| sortedFileIndex++; | ||
| return true; | ||
| } | ||
| virtual bool isValid() override { return cur != NULL; } |
system/jlib/jfile.hpp
Outdated
| @@ -632,4 +632,70 @@ const static bool filenamesAreCaseSensitive = false; | |||
| const static bool filenamesAreCaseSensitive = true; | |||
| #endif | |||
|
|
|||
| class jlib_decl CSortedDirectoryIterator : public CSimpleInterfaceOf<IDirectoryIterator> | |||
system/jlib/jfile.hpp
Outdated
| }; | ||
|
|
||
| extern jlib_decl IDirectoryIterator *getSortedDirectoryIterator(IFile *directory, SortDirectoryMode mode, bool rev, const char *mask, bool sub, bool includedirs); | ||
| extern jlib_decl IDirectoryIterator *getSortedDirectoryIterator(const char *dirName, SortDirectoryMode mode, bool rev, const char *mask, bool sub, bool includedirs); |
There was a problem hiding this comment.
probably should default params in same way as directoryFiles
|
@wangkx - a couple of comments. |
|
@jakesmith please review my changes. |
system/jlib/jfile.hpp
Outdated
| @@ -632,4 +632,7 @@ const static bool filenamesAreCaseSensitive = false; | |||
| const static bool filenamesAreCaseSensitive = true; | |||
| #endif | |||
|
|
|||
| extern jlib_decl IDirectoryIterator *getSortedDirectoryIterator(IFile *directory, SortDirectoryMode mode, bool rev, const char *mask = NULL, bool sub = false, bool includedirs = false); | |||
| extern jlib_decl IDirectoryIterator *getSortedDirectoryIterator(const char *dirName, SortDirectoryMode mode, bool rev, const char *mask = NULL, bool sub = false, bool includedirs = false); | |||
There was a problem hiding this comment.
Would make sense to default mode and rev here too, since others have defaults and common usage is going to be mode=SD_byname and rev=false
system/jlib/jfile.cpp
Outdated
| @@ -6755,3 +6755,87 @@ timestamp_type getTimeStamp(IFile * file) | |||
| file->getTime(nullptr, &modified, nullptr); | |||
| return modified.getTimeStamp(); | |||
| } | |||
|
|
|||
| class jlib_decl CSortedDirectoryIterator : public CSimpleInterfaceOf<IDirectoryIterator> | |||
There was a problem hiding this comment.
don't need/want jlib_decl here.
|
@wangkx - some minor comments. |
|
@jakesmith please review my last commit. |
|
@wangkx - looks good. @richardkchapman - ready to merge. |
|
I am going to squash the commits before merging. |
Signed-off-by: wangkx <kevin.wang@lexisnexis.com>
|
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK |
|
@richardkchapman this PR is ready to be merged. |
Signed-off-by: wangkx kevin.wang@lexisnexis.com
Type of change:
Checklist:
Testing: