r212427 - libclang: refactor handling of unsaved_files
David Blaikie
dblaikie at gmail.com
Mon Jul 7 21:26:35 PDT 2014
On Mon, Jul 7, 2014 at 9:14 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.
Without mention of the context, but that's OK (sometimes people prefer
to mention that the commit was motivated by code review, mentioning
the original revision, etc - helps provide breadcrumbs later), not
something I was commenting on.
> That
> commit was a couple of hours ago and I obviously intend to follow-up once
> remaining points are addressed..
Sorry about that - but it wasn't obvious to me. I seemed to recall
seeing this (code review feedback responded to, but lacking the trail
back to the original commit, etc) previously (perhaps from you,
perhaps others as well/instead - I don't know) & figured I'd comment
on it to help things along.
>> 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.
Not even in the slightest - wouldn't mind understanding. But if as it
pertains to the code review it might be sufficient to say "the system
here needs all the parameters wrapped in a single struct" (possibly
with a sentence about why), that doesn't seem terribly laborious, and
the code change you made seems great (the struct member types then
match what the argument types would be, if it weren't for this process
isolation mechanism/limitations).
>> 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.
No need to apologize - I was talking casually about what might be
possible, knowing very little about the limitations or how deep-seeded
or well justified they are. It's totally OK to say "yeah, it's this
way for a good reason and it's either not worth changing or worth
changing but requires a lot of work that's not a priority right now".
> 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.
Sure - I just usually at least handle one email in its entirety - no
reason that's everyone else's process. As I said above, just thought
I'd recalled some other cases of code review responses without final
feedback/breadcrumbs. I may've misremembered.
> 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.
Sure - as I said, don't really know how costly something is until I
ask - I figured you knew, so all I was asking was to understand that,
then you/I/anyone else can appropriately prioritize it.
Thanks,
- David
>
> 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