r212427 - libclang: refactor handling of unsaved_files
Alp Toker
alp at nuanti.com
Mon Jul 7 21:14:32 PDT 2014
On 08/07/2014 06:27, David Blaikie wrote:
> 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).
I've explained the fixes already in detail within the commit log. That
commit was a couple of hours ago and I obviously intend to follow-up
once remaining points are addressed..
> 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
David, It's OK to ask questions, but but it's clear from what you've
said above that you're not particularly familiar with the libclang
process isolation model.
> 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)
The message structure is passed by pointer to an isolated thread to
enable strong crash recovery. I'm sorry but we're *not* rewriting this
any time soon, and especially not as part of a commit to consolidate
unsaved_files.
You've sent me 10 emails today, often on topics that are tangential to
the actual commits. You need to be aware that these aren't going to get
same-day responses.
Meanwhile, if you want to templatize, introduce variadic/perfect
forwarding/smart pointers you can of course submit patches to the
commits list like everyone else.
Alp.
>
> 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
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list