sammccall added inline comments.
================ Comment at: lib/Sema/SemaCodeComplete.cpp:8375 + auto AddFilesFromIncludeDir = [&](StringRef IncludeDir, bool IsSystem, + bool isFramework) { + bool stripFrameworkSuffix = false; ---------------- nit: `IsFramework`, `StripFrameworkSuffix` etc ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8375 + auto AddFilesFromIncludeDir = [&](StringRef IncludeDir, bool IsSystem, + bool isFramework) { + bool stripFrameworkSuffix = false; ---------------- sammccall wrote: > nit: `IsFramework`, `StripFrameworkSuffix` etc probably clearer just to pass in the `DirectoryLookup::LookupType_t`, rather than another bool. ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8378 llvm::SmallString<128> Dir = IncludeDir; - if (!NativeRelDir.empty()) + if (isFramework) { + if (NativeRelDir.empty()) { ---------------- I don't think keeping the `stripFrameworkSuffix` state is worth scattering the reader's attention here. I'd suggest instead: ``` if (!NativeRelDir.empty()) { if (LookupType == LT_Framework) { // In a framework directory, Foo/Bar/ is actually Foo.framework/Headers/Bar/. ... } else { append(Dir, NativeRelDir); } } ``` or even pass Dir, NativeRelDir, and LookupType to a helper function to sort it out. Below, you can just check NativeRelDir.empty() && type == Framework again... If you do choose to keep it, please give it a descriptive name rather than an imperative one, e.g. `ListingFrameworksDir` ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8378 llvm::SmallString<128> Dir = IncludeDir; - if (!NativeRelDir.empty()) + if (isFramework) { + if (NativeRelDir.empty()) { ---------------- sammccall wrote: > I don't think keeping the `stripFrameworkSuffix` state is worth scattering > the reader's attention here. > > I'd suggest instead: > > ``` > if (!NativeRelDir.empty()) { > if (LookupType == LT_Framework) { > // In a framework directory, Foo/Bar/ is actually > Foo.framework/Headers/Bar/. > ... > } else { > append(Dir, NativeRelDir); > } > } > ``` > > or even pass Dir, NativeRelDir, and LookupType to a helper function to sort > it out. > > Below, you can just check NativeRelDir.empty() && type == Framework again... > > If you do choose to keep it, please give it a descriptive name rather than an > imperative one, e.g. `ListingFrameworksDir` this needs some explanation (mostly the why, not the how) ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8388 + Framework += ".framework"; + llvm::sys::path::append(Dir, Framework, "Headers"); + llvm::sys::path::append(Dir, ++Begin, End); ---------------- just `append(Dir, *Begin + ".framework", "Headers")` ? append takes twines I think. ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8390 + llvm::sys::path::append(Dir, ++Begin, End); + } else { + SmallString<32> Framework(NativeRelDir); ---------------- why is the if/else needed? the code above should cover both cases, the last append is a no-op if ++begin == end. ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8409 case llvm::sys::fs::file_type::directory_file: - AddCompletion(Filename, /*IsDirectory=*/true); + if (stripFrameworkSuffix && Filename.endswith(".framework")) { + auto FrameworkName = Filename.substr(0, Filename.size() - 10); ---------------- if a framework directory contains a subdirectory that does not end in ".framework", is it actually legal to include from it? Maybe we should be omitting it. ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8410 + if (stripFrameworkSuffix && Filename.endswith(".framework")) { + auto FrameworkName = Filename.substr(0, Filename.size() - 10); + AddCompletion(FrameworkName, /*IsDirectory=*/true); ---------------- ``` if (should strip suffix) Filename.consume_back(".framework") ``` (no need for else) ================ Comment at: lib/Sema/SemaCodeComplete.cpp:8410 + if (stripFrameworkSuffix && Filename.endswith(".framework")) { + auto FrameworkName = Filename.substr(0, Filename.size() - 10); + AddCompletion(FrameworkName, /*IsDirectory=*/true); ---------------- sammccall wrote: > ``` > if (should strip suffix) > Filename.consume_back(".framework") > ``` > (no need for else) one-line comment should be enough to explain the intent here: `// Entries in a framework directory have a .framework suffix, this does not appear in the source code` Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58062/new/ https://reviews.llvm.org/D58062 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits