erase used while iterating through the map

[fenbf]

in your code you use paths_.erase(el.first);... but that might invalidate the iterator.

probably it's better to use while loop:

auto it = paths_.begin();
while (it != paths_.end())
{
    if (!std::filesystem::exists(it->first)) {
        action(it->first, FileStatus::erased);
        it = paths_.erase(it);
    }
    else
        it++;
}

[sol-prog]

As far as I understand it, only the reference to the erased element is invalidated https://en.cppreference.com/w/cpp/container/unordered_map/erase so you can use a range for loop. What you shouldn't do is to try to the use object once erased.

Also, I pass a copy of the string path to the user defined lambda function https://github.com/sol-prog/cpp17-filewatcher/blob/master/test_fs_watcher.cpp#L15 before erasing the map element. I don't pass a reference to a map element.

Let me know if you think I'm wrong.

[fenbf]

ok, so this should work in unordered_maps:
"The order of the elements that are not erased is preserved (this makes it possible to erase individual elements while iterating through the container) "
I changed it to std::map in my forked implementation and got AV at some point.

[sol-prog]

Yes, I remember reading that I can safely erase elements from a std::unordered_map. I've also tested the code with various inputs and never had a crash.

std::map will not work for this case.

[fenbf]

sorry for bringing that up again :)
hmmm... there still might be a problem with this implementation. on VS 2017 it crashes

use case: run the app, add one file, wait till it appears in the console output as created, then delete that file and wait for the log message (app crashes)
so I had to change the implementation and use it = map.erase(it)

other sources like https://en.cppreference.com/w/cpp/container/unordered_map/erase or https://stackoverflow.com/questions/38468844/erasing-elements-from-unordered-map-in-a-loop/38469800 also mention that same pattern.

if you use erase(it) then it is invalidated and then incrementing it might give the error. Maybe in GCC/clang it behaves differently...

[sol-prog]

[sol-prog]

I would assume VS has a bug in this case if you can't erase the element while iterating.

BTW, have you tried compiling with /std:c++17 or /std:c++latest ?

[fenbf]

yes, I see, but there's also another sentence "References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated."
so if you write erase(it) then it becomes invalidated... isn't it?
I compiled with stdc++17

[sol-prog]

I may be wrong, but this is how I understand "References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated."

Say you have some unordered map {a->x,b->y,c->z,d->r} and you have a reference to the second element b. Now, if you erase b while in a loop the reference to b will be invalidated but you should still be able to finish the loop without crashing. What I'm trying to say is that the iterator to the map should still be OK to use during the loop.

I'll do some tests later today and try to reproduce your crash.

[sol-prog]

So, I was wrong :)

I did some tests with Visual Studio 2019 and you were right the app crashed when a file was removed. I can patch the code myself or you can send me a pull request if you prefer. Let me know how you want to proceed.

Thanks.

[fenbf]

it's probably faster if you provide the code, thanks!

[sol-prog]

Will do, just wanted to ask you if you had any problems with VS 2017 on this line for example:

https://github.com/sol-prog/cpp17-filewatcher/blob/master/FileWatcher.h#L50

because with VS 2019 I had to change it into something like this

if(paths_[file.path().string()] != current_file_last_write_time)

(Clang and GCC don't have any problem with the above.)

[sol-prog]

OK, I've modified the code. Thanks for suggesting a solution, I've mentioned your contribution in the pull request.

[fenbf]

ah, yes, I needed to add .string() calls as there's no implicit conversion from path to string in MSVC
why does it compile on GCC/clang?

[sol-prog]

Yep, there was no problem under GCC 8.3 and Clang 8.0 when compiled with:

-std=c++17 -Wall -Wextra -pedantic

[sol-prog]

I suppose both Clang/GCC will implicitly convert from path to string.

It seems that the only way to write truly portable C++ (or C) code is to test on all possible compiler/operating systems combinations.

(As usual, Apple's Clang still doesn't provide the filesystem header or libc++fs library.)

[fenbf]

in code there's a map of std::string and the path class has conversion operator:
https://en.cppreference.com/w/cpp/filesystem/path/native

but it converts to native string only... since GCC uses char/string for the representation then it works out of the box. On Windows we use wstring to store the path in the native format, so that's why extra conversion is needed.

so, to improve the "multiplatform'ity" it's probably better to declare map as:
std::unordered_map<filesystem::path::string_type, std::filesystem::file_time_type> paths_;

and also pass that type around rather than string. For the action callable object we could use filesystem::path, rather than string.