Hi Alp, It looks like you responded to some of this code review in r212497. Usually we follow up with a reply to the original review feedback and mention the commit message where feedback was addressed (and explain why unaddressed feedback was left as it was).
Anyway, I see you made the mutable member a reference instead. That seems OK, but still a bit 'interesting' - do you know if there's any reason the return value of clang_parseTranslationUnit_Impl wasn't used for the result, or at least a separate reference parameter? (I'm not sure why all the parameters, especially the result, were bundled into a struct - I guess it might have to do with that RunSafely function? Maybe it could be made variadic/perfect forwarding, if it isn't already) On Mon, Jul 7, 2014 at 2:11 PM, David Blaikie <[email protected]> wrote: > On Sun, Jul 6, 2014 at 6:23 PM, Alp Toker <[email protected]> wrote: >> Author: alp >> Date: Sun Jul 6 20:23:14 2014 >> New Revision: 212427 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=212427&view=rev >> Log: >> libclang: refactor handling of unsaved_files >> >> Consolidate CXUnsavedFile argument handling in API functions to support a >> wider >> cleanup of various file remapping schemes in the frontend. >> >> Modified: >> cfe/trunk/tools/libclang/CIndex.cpp >> cfe/trunk/tools/libclang/CIndexCodeCompletion.cpp >> cfe/trunk/tools/libclang/CXString.h >> cfe/trunk/tools/libclang/Indexing.cpp >> >> Modified: cfe/trunk/tools/libclang/CIndex.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndex.cpp?rev=212427&r1=212426&r2=212427&view=diff >> ============================================================================== >> --- cfe/trunk/tools/libclang/CIndex.cpp (original) >> +++ cfe/trunk/tools/libclang/CIndex.cpp Sun Jul 6 20:23:14 2014 >> @@ -2727,21 +2727,18 @@ struct ParseTranslationUnitInfo { >> const char *source_filename; >> const char *const *command_line_args; >> int num_command_line_args; >> - struct CXUnsavedFile *unsaved_files; >> - unsigned num_unsaved_files; >> + ArrayRef<CXUnsavedFile> unsaved_files; >> unsigned options; >> CXTranslationUnit *out_TU; >> - CXErrorCode result; >> + mutable CXErrorCode result; > > I haven't looked at the code in detail - but is there a one-sentence > summary of why the right pivot/design here is to have this field > mutable? (& similarly for another field or two that are becoming > mutable in this change) > >> }; >> static void clang_parseTranslationUnit_Impl(void *UserData) { >> - ParseTranslationUnitInfo *PTUI = >> - static_cast<ParseTranslationUnitInfo*>(UserData); >> + const ParseTranslationUnitInfo *PTUI = >> + static_cast<ParseTranslationUnitInfo *>(UserData); > > If you like, you could add "const" to the cast and just use "auto *" > (or even plain "auto") on the LHS. (& similarly with some other > changes in this commit) > >> CXIndex CIdx = PTUI->CIdx; >> const char *source_filename = PTUI->source_filename; >> const char * const *command_line_args = PTUI->command_line_args; >> int num_command_line_args = PTUI->num_command_line_args; >> - struct CXUnsavedFile *unsaved_files = PTUI->unsaved_files; >> - unsigned num_unsaved_files = PTUI->num_unsaved_files; >> unsigned options = PTUI->options; >> CXTranslationUnit *out_TU = PTUI->out_TU; >> >> @@ -2751,8 +2748,7 @@ static void clang_parseTranslationUnit_I >> PTUI->result = CXError_Failure; >> >> // Check arguments. >> - if (!CIdx || !out_TU || >> - (unsaved_files == nullptr && num_unsaved_files != 0)) { >> + if (!CIdx || !out_TU) { >> PTUI->result = CXError_InvalidArguments; >> return; >> } >> @@ -2789,12 +2785,10 @@ static void clang_parseTranslationUnit_I >> llvm::CrashRecoveryContextCleanupRegistrar< >> std::vector<ASTUnit::RemappedFile> > >> RemappedCleanup(RemappedFiles.get()); >> >> - for (unsigned I = 0; I != num_unsaved_files; ++I) { >> - StringRef Data(unsaved_files[I].Contents, unsaved_files[I].Length); >> - llvm::MemoryBuffer *Buffer = >> - llvm::MemoryBuffer::getMemBufferCopy(Data, >> unsaved_files[I].Filename); >> - RemappedFiles->push_back(std::make_pair(unsaved_files[I].Filename, >> - Buffer)); >> + for (auto &UF : PTUI->unsaved_files) { >> + llvm::MemoryBuffer *MB = >> + llvm::MemoryBuffer::getMemBufferCopy(getContents(UF), UF.Filename); >> + RemappedFiles->push_back(std::make_pair(UF.Filename, MB)); >> } >> >> std::unique_ptr<std::vector<const char *>> Args( >> @@ -2895,10 +2889,18 @@ enum CXErrorCode clang_parseTranslationU >> *Log << command_line_args[i] << " "; >> } >> >> - ParseTranslationUnitInfo PTUI = { CIdx, source_filename, >> command_line_args, >> - num_command_line_args, unsaved_files, >> - num_unsaved_files, options, out_TU, >> - CXError_Failure }; >> + if (num_unsaved_files && !unsaved_files) >> + return CXError_InvalidArguments; >> + >> + ParseTranslationUnitInfo PTUI = { >> + CIdx, >> + source_filename, >> + command_line_args, >> + num_command_line_args, >> + llvm::makeArrayRef(unsaved_files, num_unsaved_files), >> + options, >> + out_TU, >> + CXError_Failure}; >> llvm::CrashRecoveryContext CRC; >> >> if (!RunSafely(CRC, clang_parseTranslationUnit_Impl, &PTUI)) { >> @@ -3029,20 +3031,17 @@ unsigned clang_defaultReparseOptions(CXT >> >> struct ReparseTranslationUnitInfo { >> CXTranslationUnit TU; >> - unsigned num_unsaved_files; >> - struct CXUnsavedFile *unsaved_files; >> + ArrayRef<CXUnsavedFile> unsaved_files; >> unsigned options; >> - int result; >> + mutable int result; >> }; >> >> static void clang_reparseTranslationUnit_Impl(void *UserData) { >> - ReparseTranslationUnitInfo *RTUI = >> - static_cast<ReparseTranslationUnitInfo*>(UserData); >> + const ReparseTranslationUnitInfo *RTUI = >> + static_cast<ReparseTranslationUnitInfo *>(UserData); >> RTUI->result = CXError_Failure; >> >> CXTranslationUnit TU = RTUI->TU; >> - unsigned num_unsaved_files = RTUI->num_unsaved_files; >> - struct CXUnsavedFile *unsaved_files = RTUI->unsaved_files; >> unsigned options = RTUI->options; >> (void) options; >> >> @@ -3052,10 +3051,6 @@ static void clang_reparseTranslationUnit >> RTUI->result = CXError_InvalidArguments; >> return; >> } >> - if (unsaved_files == nullptr && num_unsaved_files != 0) { >> - RTUI->result = CXError_InvalidArguments; >> - return; >> - } >> >> // Reset the associated diagnostics. >> delete static_cast<CXDiagnosticSetImpl*>(TU->Diagnostics); >> @@ -3074,13 +3069,11 @@ static void clang_reparseTranslationUnit >> // Recover resources if we crash before exiting this function. >> llvm::CrashRecoveryContextCleanupRegistrar< >> std::vector<ASTUnit::RemappedFile> > >> RemappedCleanup(RemappedFiles.get()); >> - >> - for (unsigned I = 0; I != num_unsaved_files; ++I) { >> - StringRef Data(unsaved_files[I].Contents, unsaved_files[I].Length); >> - llvm::MemoryBuffer *Buffer = >> - llvm::MemoryBuffer::getMemBufferCopy(Data, >> unsaved_files[I].Filename); >> - RemappedFiles->push_back(std::make_pair(unsaved_files[I].Filename, >> - Buffer)); >> + >> + for (auto &UF : RTUI->unsaved_files) { >> + llvm::MemoryBuffer *MB = >> + llvm::MemoryBuffer::getMemBufferCopy(getContents(UF), UF.Filename); >> + RemappedFiles->push_back(std::make_pair(UF.Filename, MB)); >> } >> >> if (!CXXUnit->Reparse(*RemappedFiles.get())) >> @@ -3097,8 +3090,12 @@ int clang_reparseTranslationUnit(CXTrans >> *Log << TU; >> } >> >> - ReparseTranslationUnitInfo RTUI = { TU, num_unsaved_files, unsaved_files, >> - options, CXError_Failure }; >> + if (num_unsaved_files && !unsaved_files) >> + return CXError_InvalidArguments; >> + >> + ReparseTranslationUnitInfo RTUI = { >> + TU, llvm::makeArrayRef(unsaved_files, num_unsaved_files), options, >> + CXError_Failure}; >> >> if (getenv("LIBCLANG_NOTHREADS")) { >> clang_reparseTranslationUnit_Impl(&RTUI); >> >> Modified: cfe/trunk/tools/libclang/CIndexCodeCompletion.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndexCodeCompletion.cpp?rev=212427&r1=212426&r2=212427&view=diff >> ============================================================================== >> --- cfe/trunk/tools/libclang/CIndexCodeCompletion.cpp (original) >> +++ cfe/trunk/tools/libclang/CIndexCodeCompletion.cpp Sun Jul 6 20:23:14 >> 2014 >> @@ -651,8 +651,7 @@ struct CodeCompleteAtInfo { >> const char *complete_filename; >> unsigned complete_line; >> unsigned complete_column; >> - struct CXUnsavedFile *unsaved_files; >> - unsigned num_unsaved_files; >> + ArrayRef<CXUnsavedFile> unsaved_files; >> unsigned options; >> CXCodeCompleteResults *result; >> }; >> @@ -662,8 +661,6 @@ void clang_codeCompleteAt_Impl(void *Use >> const char *complete_filename = CCAI->complete_filename; >> unsigned complete_line = CCAI->complete_line; >> unsigned complete_column = CCAI->complete_column; >> - struct CXUnsavedFile *unsaved_files = CCAI->unsaved_files; >> - unsigned num_unsaved_files = CCAI->num_unsaved_files; >> unsigned options = CCAI->options; >> bool IncludeBriefComments = options & CXCodeComplete_IncludeBriefComments; >> CCAI->result = nullptr; >> @@ -693,14 +690,13 @@ void clang_codeCompleteAt_Impl(void *Use >> >> // Perform the remapping of source files. >> SmallVector<ASTUnit::RemappedFile, 4> RemappedFiles; >> - for (unsigned I = 0; I != num_unsaved_files; ++I) { >> - StringRef Data(unsaved_files[I].Contents, unsaved_files[I].Length); >> - llvm::MemoryBuffer *Buffer = >> - llvm::MemoryBuffer::getMemBufferCopy(Data, >> unsaved_files[I].Filename); >> - RemappedFiles.push_back(std::make_pair(unsaved_files[I].Filename, >> - Buffer)); >> + >> + for (auto &UF : CCAI->unsaved_files) { >> + llvm::MemoryBuffer *MB = >> + llvm::MemoryBuffer::getMemBufferCopy(getContents(UF), UF.Filename); >> + RemappedFiles.push_back(std::make_pair(UF.Filename, MB)); >> } >> - >> + >> if (EnableLogging) { >> // FIXME: Add logging. >> } >> @@ -823,9 +819,12 @@ CXCodeCompleteResults *clang_codeComplet >> << complete_filename << ':' << complete_line << ':' << >> complete_column; >> } >> >> - CodeCompleteAtInfo CCAI = { TU, complete_filename, complete_line, >> - complete_column, unsaved_files, >> num_unsaved_files, >> - options, nullptr }; >> + if (num_unsaved_files && !unsaved_files) >> + return nullptr; >> + >> + CodeCompleteAtInfo CCAI = {TU, complete_filename, complete_line, >> + complete_column, llvm::makeArrayRef(unsaved_files, num_unsaved_files), >> + options, nullptr}; >> >> if (getenv("LIBCLANG_NOTHREADS")) { >> clang_codeCompleteAt_Impl(&CCAI); >> >> Modified: cfe/trunk/tools/libclang/CXString.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CXString.h?rev=212427&r1=212426&r2=212427&view=diff >> ============================================================================== >> --- cfe/trunk/tools/libclang/CXString.h (original) >> +++ cfe/trunk/tools/libclang/CXString.h Sun Jul 6 20:23:14 2014 >> @@ -97,6 +97,10 @@ CXStringBuf *getCXStringBuf(CXTranslatio >> bool isManagedByPool(CXString str); >> >> } >> + >> +static inline StringRef getContents(const CXUnsavedFile &UF) { >> + return StringRef(UF.Contents, UF.Length); >> +} >> } >> >> #endif >> >> Modified: cfe/trunk/tools/libclang/Indexing.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/Indexing.cpp?rev=212427&r1=212426&r2=212427&view=diff >> ============================================================================== >> --- cfe/trunk/tools/libclang/Indexing.cpp (original) >> +++ cfe/trunk/tools/libclang/Indexing.cpp Sun Jul 6 20:23:14 2014 >> @@ -472,28 +472,17 @@ struct IndexSourceFileInfo { >> const char *source_filename; >> const char *const *command_line_args; >> int num_command_line_args; >> - struct CXUnsavedFile *unsaved_files; >> - unsigned num_unsaved_files; >> + ArrayRef<CXUnsavedFile> unsaved_files; >> CXTranslationUnit *out_TU; >> unsigned TU_options; >> - int result; >> -}; >> - >> -struct MemBufferOwner { >> - SmallVector<const llvm::MemoryBuffer *, 8> Buffers; >> - >> - ~MemBufferOwner() { >> - for (SmallVectorImpl<const llvm::MemoryBuffer *>::iterator >> - I = Buffers.begin(), E = Buffers.end(); I != E; ++I) >> - delete *I; >> - } >> + mutable int result; >> }; >> >> } // anonymous namespace >> >> static void clang_indexSourceFile_Impl(void *UserData) { >> - IndexSourceFileInfo *ITUI = >> - static_cast<IndexSourceFileInfo*>(UserData); >> + const IndexSourceFileInfo *ITUI = >> + static_cast<IndexSourceFileInfo *>(UserData); >> CXIndexAction cxIdxAction = ITUI->idxAction; >> CXClientData client_data = ITUI->client_data; >> IndexerCallbacks *client_index_callbacks = ITUI->index_callbacks; >> @@ -502,8 +491,6 @@ static void clang_indexSourceFile_Impl(v >> const char *source_filename = ITUI->source_filename; >> const char * const *command_line_args = ITUI->command_line_args; >> int num_command_line_args = ITUI->num_command_line_args; >> - struct CXUnsavedFile *unsaved_files = ITUI->unsaved_files; >> - unsigned num_unsaved_files = ITUI->num_unsaved_files; >> CXTranslationUnit *out_TU = ITUI->out_TU; >> unsigned TU_options = ITUI->TU_options; >> >> @@ -584,18 +571,18 @@ static void clang_indexSourceFile_Impl(v >> if (CInvok->getFrontendOpts().Inputs.empty()) >> return; >> >> - std::unique_ptr<MemBufferOwner> BufOwner(new MemBufferOwner()); >> + typedef SmallVector<std::unique_ptr<llvm::MemoryBuffer>, 8> >> MemBufferOwner; >> + std::unique_ptr<MemBufferOwner> BufOwner(new MemBufferOwner); >> >> // Recover resources if we crash before exiting this method. >> - llvm::CrashRecoveryContextCleanupRegistrar<MemBufferOwner> >> - BufOwnerCleanup(BufOwner.get()); >> + llvm::CrashRecoveryContextCleanupRegistrar<MemBufferOwner> >> BufOwnerCleanup( >> + BufOwner.get()); >> >> - for (unsigned I = 0; I != num_unsaved_files; ++I) { >> - StringRef Data(unsaved_files[I].Contents, unsaved_files[I].Length); >> - llvm::MemoryBuffer *Buffer = >> - llvm::MemoryBuffer::getMemBufferCopy(Data, >> unsaved_files[I].Filename); >> - >> CInvok->getPreprocessorOpts().addRemappedFile(unsaved_files[I].Filename, >> Buffer); >> - BufOwner->Buffers.push_back(Buffer); >> + for (auto &UF : ITUI->unsaved_files) { >> + llvm::MemoryBuffer *MB = >> + llvm::MemoryBuffer::getMemBufferCopy(getContents(UF), UF.Filename); >> + BufOwner->push_back(std::unique_ptr<llvm::MemoryBuffer>(MB)); >> + CInvok->getPreprocessorOpts().addRemappedFile(UF.Filename, MB); >> } >> >> // Since libclang is primarily used by batch tools dealing with >> @@ -992,12 +979,22 @@ int clang_indexSourceFile(CXIndexAction >> *Log << command_line_args[i] << " "; >> } >> >> - IndexSourceFileInfo ITUI = { idxAction, client_data, index_callbacks, >> - index_callbacks_size, index_options, >> - source_filename, command_line_args, >> - num_command_line_args, unsaved_files, >> - num_unsaved_files, out_TU, TU_options, >> - CXError_Failure }; >> + if (num_unsaved_files && !unsaved_files) >> + return CXError_InvalidArguments; >> + >> + IndexSourceFileInfo ITUI = { >> + idxAction, >> + client_data, >> + index_callbacks, >> + index_callbacks_size, >> + index_options, >> + source_filename, >> + command_line_args, >> + num_command_line_args, >> + llvm::makeArrayRef(unsaved_files, num_unsaved_files), >> + out_TU, >> + TU_options, >> + CXError_Failure}; >> >> if (getenv("LIBCLANG_NOTHREADS")) { >> clang_indexSourceFile_Impl(&ITUI); >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
