Page MenuHomePhabricator

[PATCH][VFS] Ignore broken symlinks in the directory iterator.
ClosedPublic

Authored by ributzka on Mar 8 2017, 5:17 PM.

Details

Summary

The VFS directory iterator and recursive directory iterator behave differently
from the LLVM counterparts. Once the VFS iterators hit a broken symlink they
immediately abort. The LLVM counterparts allow to recover from this issue by
clearing the error code and skipping to the next entry.

This change adds the same functionality to the VFS iterators. There should be
no change in current behavior in the current CLANG source base, because all
clients have loop exit conditions that also check the error code.

Diff Detail

Repository
rL LLVM

Event Timeline

ributzka created this revision.Mar 8 2017, 5:17 PM
bruno edited edge metadata.Mar 9 2017, 11:54 AM

Hi Juergen,

Thanks for working on this.

include/clang/Basic/VirtualFileSystem.h
164 ↗(On Diff #91113)

I would rather we don't drop checks for ECs - it's commonly assumed that whenever they are passed in we wanna check them for errors, can't you just skip the specific case you want to avoid?

lib/Basic/VirtualFileSystem.cpp
1860 ↗(On Diff #91113)

Same here.

1873 ↗(On Diff #91113)

Can you add a comment explaining why you are doing it? I would prefer if we reset the EC state here than having the callers ignoring EC results.

ributzka added inline comments.Mar 9 2017, 2:28 PM
lib/Basic/VirtualFileSystem.cpp
1873 ↗(On Diff #91113)

If we reset the EC here, then the caller won't know that there was an issue. The idea is that we still want the caller to check EC. It should be up to the caller to decide how to act on this particular error.

I guess since the caller has to check for the error anyway I could even remove this check completely and not check EC at all here.

ributzka added inline comments.Mar 9 2017, 4:39 PM
include/clang/Basic/VirtualFileSystem.h
164 ↗(On Diff #91113)

EC is an output parameter here. The client is supposed to check it.

ributzka updated this revision to Diff 91371.Mar 10 2017, 10:01 AM

Remove the EC check completely.

bruno accepted this revision.Mar 10 2017, 1:12 PM
bruno added inline comments.
lib/Basic/VirtualFileSystem.cpp
1873 ↗(On Diff #91113)

Right, this should be fine with the latest changes and per discussions offline. LGTM

This revision is now accepted and ready to land.Mar 10 2017, 1:12 PM
This revision was automatically updated to reflect the committed changes.

Thanks Bruno. Committed in r297510.