[libcxx] r273034 - Add Filesystem TS -- Complete
Adrian Prantl via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 4 08:03:20 PDT 2016
> On Aug 3, 2016, at 1:56 PM, Bruno Cardoso Lopes <bruno.cardoso at gmail.com> wrote:
>
> Hi Eric,
>
> After we upgraded our green dragon bots to El Captain (10.11), the
> test below started to fail on some of our machines:
>
> -- test/std/experimental/filesystem/fs.op.funcs/fs.op.hard_lk_ct/hard_link_count.pass.cpp
>
> TEST_CASE(hard_link_count_for_directory)
> {
> uintmax_t DirExpect = 3;
> uintmax_t Dir3Expect = 2;
> #if defined(__APPLE__)
> DirExpect += 2;
> Dir3Expect += 1;
> #endif
Just as a general code review comment: When committing a platform-specific workaround, I would expect there to be a comment explaining what bug/peculiarity of the platform is being worked around :-)
thanks,
Adrian
> TEST_CHECK(hard_link_count(StaticEnv::Dir) == DirExpect);
> TEST_CHECK(hard_link_count(StaticEnv::Dir3) == Dir3Expect);
>
> std::error_code ec;
> TEST_CHECK(hard_link_count(StaticEnv::Dir, ec) == DirExpect);
> TEST_CHECK(hard_link_count(StaticEnv::Dir3, ec) == Dir3Expect);
> }
>
> You can see the issue in every recent (past 10 days) run on
> http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-expensive/433
>
> I noticed that commenting out the snippet around "#if
> defined(__APPLE__)" would make it work. What's the rationale behind
> the "#if defined(__APPLE__)" above?
>
> Thanks,
>
>
> On Tue, Jun 21, 2016 at 3:03 PM, Eric Fiselier via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>> The issue should be fixed in r273323.
>>
>> Thanks for the report and for your patience.
>>
>> /Eric
>>
>> On Mon, Jun 20, 2016 at 11:27 PM, Eric Fiselier <eric at efcs.ca> wrote:
>>>
>>> Hi Artem,
>>>
>>> Sorry for the delay, I've been busy with school all day.
>>> I'll check in a fix tomorrow morning.
>>>
>>> Sorry again,
>>>
>>> /Eric
>>>
>>> On Mon, Jun 20, 2016 at 2:31 PM, Eric Fiselier <eric at efcs.ca> wrote:
>>>>
>>>> Oh shoot, I definitely didn't take that into account. I'll put together a
>>>> fix.
>>>>
>>>> /Eric
>>>>
>>>>
>>>>
>>>> On Mon, Jun 20, 2016 at 2:27 PM, Artem Belevich <tra at google.com> wrote:
>>>>>
>>>>> Eric,
>>>>>
>>>>> Some tests appear to fail if the path to the tests' current directory
>>>>> has some symlinks in it.
>>>>> In my case source and build tree are in directory 'work' that's
>>>>> symlinked to from my home directory:
>>>>> /usr/local/home/tra/work -> /work/tra
>>>>>
>>>>> This causes multiple failures in libcxx tests. One example:
>>>>>
>>>>>
>>>>> projects/libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.rename/rename.pass.cpp:121
>>>>> 121 TEST_CHECK(read_symlink(bad_sym_dest) == dne);
>>>>>
>>>>> dne:
>>>>>
>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/dne
>>>>> bad_sym_dest:
>>>>>
>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/bad_sym2
>>>>>
>>>>> These are the paths that traverse the 'work' symlink in my home
>>>>> directory. However, the symlink that we're reading contains physical path to
>>>>> the directory. While it does link to the right place, it's not the path the
>>>>> test expects.
>>>>>
>>>>> #readlink
>>>>> /usr/local/home/tra/work/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/bad_sym2
>>>>>
>>>>> /work/tra/llvm/build/gpu/debug/projects/libcxx/test/filesystem/Output/dynamic_env/test.529143/dne
>>>>>
>>>>> I think we need to normalize paths before we compare them.
>>>>>
>>>>> --Artem
>>>>>
>>>>>
>>>>> On Sat, Jun 18, 2016 at 12:03 PM, Eric Fiselier via cfe-commits
>>>>> <cfe-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> I assume the correct way to fix this is to disable
>>>>>>> -Wcovered-switch-default while compiling
>>>>>>> libcxx/src/experimental/filesystem/operations.cpp
>>>>>>
>>>>>> Agreed. Disabled in r273092.
>>>>>>
>>>>>> Thanks for your patience with this latest change,
>>>>>>
>>>>>> /Eric
>>>>>>
>>>>>> On Sat, Jun 18, 2016 at 12:54 PM, Adrian Prantl <aprantl at apple.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hello Eric,
>>>>>>>
>>>>>>> this commit causes new warnings on our bots:
>>>>>>>
>>>>>>> clang/src/projects/libcxx/include/fstream:816:5: warning: default
>>>>>>> label in switch which covers all enumeration values
>>>>>>> [-Wcovered-switch-default]
>>>>>>> default:
>>>>>>>
>>>>>>> The problem is with this defensive default statement in fstream:
>>>>>>>
>>>>>>>
>>>>>>> template <class _CharT, class _Traits>
>>>>>>> 0792 typename basic_filebuf<_CharT, _Traits>::pos_type
>>>>>>> 0793 basic_filebuf<_CharT, _Traits>::seekoff(off_type __off,
>>>>>>> ios_base::seekdir __way,
>>>>>>> 0794 ios_base::openmode)
>>>>>>> 0795 {
>>>>>>> 0796 #ifndef _LIBCPP_NO_EXCEPTIONS
>>>>>>> 0797 if (!__cv_)
>>>>>>> 0798 throw bad_cast();
>>>>>>> 0799 #endif
>>>>>>> 0800 int __width = __cv_->encoding();
>>>>>>> 0801 if (__file_ == 0 || (__width <= 0 && __off != 0) || sync())
>>>>>>> 0802 return pos_type(off_type(-1));
>>>>>>> 0803 // __width > 0 || __off == 0
>>>>>>> 0804 int __whence;
>>>>>>> 0805 switch (__way)
>>>>>>> 0806 {
>>>>>>> 0807 case ios_base::beg:
>>>>>>> 0808 __whence = SEEK_SET;
>>>>>>> 0809 break;
>>>>>>> 0810 case ios_base::cur:
>>>>>>> 0811 __whence = SEEK_CUR;
>>>>>>> 0812 break;
>>>>>>> 0813 case ios_base::end:
>>>>>>> 0814 __whence = SEEK_END;
>>>>>>> 0815 break;
>>>>>>> 0816 default:
>>>>>>> 0817 return pos_type(off_type(-1));
>>>>>>> 0818 }
>>>>>>>
>>>>>>>
>>>>>>> I assume the correct way to fix this is to disable
>>>>>>> -Wcovered-switch-default while compiling
>>>>>>> libcxx/src/experimental/filesystem/operations.cpp
>>>>>>>
>>>>>>> Could you please investigate?
>>>>>>>
>>>>>>> thanks,
>>>>>>> Adrian
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> --Artem Belevich
>>>>
>>>>
>>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
>
> --
> Bruno Cardoso Lopes
> http://www.brunocardoso.cc
More information about the cfe-commits
mailing list