r212427 - libclang: refactor handling of unsaved_files
Alp Toker
alp at nuanti.com
Wed Jul 16 16:54:24 PDT 2014
On 16/07/2014 20:59, David Blaikie wrote:
> 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?
The other structures in the libclang C API still pass the error code
back by value. That means we now have two different return techniques in
deployment.
It doesn't bug me too much -- just pointing out the downside of fixing
touched lines without addressing the module as a whole.
>
>>
>>
>>>> };
>>>> 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.
Thanks for the suggestion
>
> - 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
>>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list