Skip to content

Commit

Permalink
Merge pull request #496 from CoolProp/directory_size
Browse files Browse the repository at this point in the history 
Implement checking of directory size
  • Loading branch information
@ibell
ibell committed Feb 24, 2015
2 parents 6d39bf8 + d12a488 commit e5b66b3
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 15 deletions.
5 changes: 4 additions & 1 deletion CMakeLists.txt
View file
Expand Up @@ -9,7 +9,7 @@ else()
message(STATUS "COOLPROP_INSTALL_PREFIX=${COOLPROP_INSTALL_PREFIX}")
endif()

set(CMAKE_INSTALL_PREFIX ${COOLPROP_INSTALL_PREFIX} CACHE PATH "default install path" FORCE)
set(CMAKE_INSTALL_PREFIX ${COOLPROP_INSTALL_PREFIX} CACHE PATH "default install path" FORCE)

#######################################
# PROJECT INFORMATION #
Expand Down Expand Up @@ -65,6 +65,9 @@ option (FORCE_BITNESS_32
option (FORCE_BITNESS_64
"Force a 64bit build regardless of the host"
OFF)

add_definitions(-D_UNICODE)
add_definitions(-DUNICODE)

#######################################
# FIND ALL SOURCES #
Expand Down
82 changes: 69 additions & 13 deletions include/CoolPropTools.h
View file
Expand Up @@ -5,11 +5,28 @@

#include "PlatformDetermination.h"
#include "Exceptions.h"

#include <string>
#include <vector>
#include <cmath>
#include "float.h"
#include <iterator>
#include <algorithm>
#include <functional>
#include <cctype>
#include <map>
#include <locale>
#include <fstream>
#include <cerrno>
#include <numeric>
#include <set>

#if defined(__ISWINDOWS__)
#include "Windows.h"
#endif

// Always undef these stupid macros
#undef min
#undef max

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 24, 2015

Contributor

Why don't you define NOMINMAX? See minwindef.h. See http://stackoverflow.com/questions/4913922/possible-problems-with-nominmax-on-visual-c
I would suggest defining it as compiler option, to cover all compilation units, no matter what: -DNOMINMAX

Also, using syntax like (std::min)(a,b) will prevent using macros. But this is the local solution...

This comment has been minimized.

Sign in to view

Copy link
@ibell

ibell via email Feb 24, 2015

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 24, 2015

Contributor

Why does (std::min)(a,b) not allow for macro expansion?

Because for macro function to expand, it must be followed by an optional space and opening parenthesis, not closing one.

This comment has been minimized.

Sign in to view

Copy link
@jowr

jowr Feb 24, 2015

Member

I think, I cannot follow, but I really do appreciate that you spend so much time reviewing our code. I sleep better knowing that someone who actually knows C++ improves the code. As you might have noticed, we are a bunch of self-educated programmers that write software in a language no one ever taught us to use properly...

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 24, 2015

Contributor

Well, it's my poor English.
I'll try again, hope I'll describe properly.

Assume a macro:

define min(a,b) (((a)<(b)) ? (a) : (b))

and a function definition

int min (int a, int b);

Then this will expand as macro at the preprocessing:

x=min(y,z);

i.e. it'll become for compiler

x=(((y)<(z)) ? (y) : (z))

But if you use

x=(min)(y,z)

then preprocessor sees the min, sees the definition of the macro, and knows that it must be followed by (. But it isn't followed by opening parenthesis (, but by closing parenthesis ), so preprocessor doesn't replace "min" with its macro expansion, and compiler gets its (min)(y,z) unchanged.

This comment has been minimized.

Sign in to view

Copy link
@jowr

jowr Feb 24, 2015

Member

That makes sense, thank you for the clarification.

#ifndef M_PI
# define M_PI 3.14159265358979323846
Expand Down Expand Up @@ -66,8 +83,6 @@
#endif

#if defined(__powerpc__)
#undef min
#undef max
#undef EOS
#endif

Expand All @@ -86,17 +101,58 @@
#pragma message("WARNING: You need to implement DEPRECATED for this compiler")
#define DEPRECATED(func) func
#endif

/// Copy string to wstring
/// Dangerous if the string has non-ASCII characters; from http://stackoverflow.com/a/8969776/1360263
inline void StringToWString(const std::string &s, std::wstring &ws)

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 24, 2015

Contributor

This is DEFINITELY a time bomb. Only 127-bit characters would be converted, and I, for inctance, won't be able to use, say, russian directory names. I know of projects that standardize on UTF8 internally, and convert to it on input from foreign API, and convert from it appropriately to output to foreign API. I would suggest a couple of functions to do that universally, that would take a char* and convert them to UTF-8 encoded std::string (using system locale or a user-provided locale); and take UTF-8 encoded std::string and convert to char*. And another couple to convert from/to std:wstring.

I'll make something like that for your consideration.
{
ws = std::wstring(s.begin(), s.end());
}

#include <iterator>
#include <algorithm>
#include <functional>
#include <cctype>
#include <map>
#include <locale>
#include <fstream>
#include <cerrno>
#include <numeric>
#include <set>
#if defined(__ISWINDOWS__)
/// From http://stackoverflow.com/a/17827724/1360263
inline bool IsBrowsePath(const std::wstring& path)
{
return (path == L"." || path == L"..");
}
inline unsigned long long CalculateDirSize(const std::wstring &path, std::vector<std::wstring> *errVect = NULL, unsigned long long size = 0)
{
WIN32_FIND_DATA data;
HANDLE sh = NULL;
sh = FindFirstFile((path + L"\\*").c_str(), &data);

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 25, 2015

Contributor

When you use WinAPI like this (that has A and W versions, and a macro that depend on UNICODE def), and you explicitly pass wchar_t or char, not TCHARs, then it's MUCH BETTER to use widechar API explicitly:

WIN32_FIND_DATAW ...
FindFirstFileW(...

even if you define global UNICODE flag. It makes things self-documenting.

if (sh == INVALID_HANDLE_VALUE )
{
//if we want, store all happened error
if (errVect != NULL)
errVect ->push_back(path);
return size;
}

do
{
// skip current and parent
if (!IsBrowsePath(data.cFileName))
{
// if found object is ...
if ((data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == FILE_ATTRIBUTE_DIRECTORY)
// directory, then search it recursievly
size = CalculateDirSize(path + L"\\" + data.cFileName, NULL, size);

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 25, 2015

Contributor

I don't understand the purpose to pass size as param, and assign the return value back.

  1. Allocate space on stack, copy size to it
  2. After calculations, copy size to temporary return value of function and to size itself...
    Personally I would remove the parameter, initialize size to zero at the beginning, and change this line to

size += CalculateDirSize(path + L"" + data.cFileName, NULL);
else
// otherwise get object size and add it to directory size
size += (unsigned long long) (data.nFileSizeHigh * (MAXDWORD ) + data.nFileSizeLow);

This comment has been minimized.

Sign in to view

Copy link
@mikekaganski

mikekaganski Feb 25, 2015

Contributor

I'm not sure, but I'm afraid that this may produce erroneous results:
first, (data.nFileSizeHigh * (MAXDWORD ) + data.nFileSizeLow) is calculated, and as it only contain DWORDs, it will store results of each sub-operation as 32-bit integer, trimming the data.nFileSizeHigh * (MAXDWORD ) to zeroes.
Second, it will do the conversion of result to unsigned long long, where it will only deal with data.nFileSizeLow.

I feel it better to use something like this:
size += data.nFileSizeHigh * (unsigned long long)(MAXDWORD ) + data.nFileSizeLow;

Here one operand is explicitly unsigned long long, thus all the expression is promoted to that type.

This comment has been minimized.

Sign in to view

Copy link
@ibell

ibell via email Feb 25, 2015

Author Contributor
}

} while (FindNextFile(sh, &data)); // do

FindClose(sh);

return size;
}
#else
/// Get the size of a directory in bytes
unsigned long long CalculateDirSize(const std::string &path);
#endif

/// The following code for the trim functions was taken from http://stackoverflow.com/questions/216823/whats-the-best-way-to-trim-stdstring
// trim from start
Expand Down
20 changes: 20 additions & 0 deletions src/CoolPropTools.cpp
View file
Expand Up @@ -12,6 +12,26 @@
#include "MatrixMath.h"
#include "Exceptions.h"

#if !defined(__ISWINDOWS__)
#include <ftw.h>
#include <stdint.h>
#include <iostream>

static double ftw_summer; // An evil global variable for the ftw function
int ftw_function(const char *fpath, const struct stat *sb, int tflag, struct FTW *ftwbuf){
ftw_summer += sb->st_size;
return 0; /* To tell nftw() to continue */
}
unsigned long long CalculateDirSize(const std::string &path){
ftw_summer = 0;
int flags = 0 | FTW_DEPTH | FTW_PHYS;
nftw(path.c_str(), ftw_function, 20, flags);
double temp = ftw_summer;
ftw_summer = 0;
return temp;
}
#endif

double root_sum_square(const std::vector<double> &x)
{
double sum = 0;
Expand Down
3 changes: 2 additions & 1 deletion wrappers/Python/setup.py
View file
Expand Up @@ -222,7 +222,8 @@ def find_cpp_sources(root = os.path.join('..','..','src'), extensions = ['.cpp']
sys.argv += ['clean', 'install']

common_args = dict(include_dirs = include_dirs,
language='c++')
language='c++',
extra_compile_args=['-DUNICODE','-D_UNICODE'])

if USE_CYTHON:
common_args.update(dict(cython_c_in_temp = True,
Expand Down

0 comments on commit e5b66b3

Please sign in to comment.