r212427 - libclang: refactor handling of unsaved_files
David Blaikie
dblaikie at gmail.com
Wed Jul 16 10:59:19 PDT 2014
On Thu, Jul 10, 2014 at 7:05 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 08/07/2014 00:11, David Blaikie 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)
>
>
> Fixed in r212497 (libclang: pass return code out argument by reference).
>
> We now have const-correctness in the touched functions, at the cost of
> having deviated them away from the convention used in all the other C
> wrapper functions in the module.
Which consistency are you referring to here?
>
>
>
>>
>>> };
>>> 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)
>
>
> There's so much we could do here -- those lines can be effectively factored
> away and there's no need to copy the fields into locals one by one in the
> chunks below.
Sure, was just a simple suggestion that, since you were touching the
line, you could remove the redundancy in the process of adding the
desired const. Could be a useful tip/thought for future similar
changes.
- David
>
> Alp.
>
>
>
>>
>>> 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
>
>
> --
> http://www.nuanti.com
> the browser experts
>
More information about the cfe-commits
mailing list