r212427 - libclang: refactor handling of unsaved_files
David Blaikie
dblaikie at gmail.com
Mon Jul 7 20:27:09 PDT 2014
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 <dblaikie at gmail.com> wrote:
> On Sun, Jul 6, 2014 at 6:23 PM, Alp Toker <alp at nuanti.com> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list