Author: Alp Toker <[email protected]>
Date:   Mon Jul 7 22:42:03 2014 +0000

    libclang: pass return code out argument by reference

    r212427 formalized the message-passing pattern by making these argument
    structures const. This commit changes output arguments to get passed by
    reference so we can eliminate mutable fields.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@212497 91177308-0d34-0410-b5e6-96231b3b80d8


On 08/07/2014 07:26, David Blaikie wrote:
On Mon, Jul 7, 2014 at 9:14 PM, Alp Toker <[email protected]> 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

See the commit log in question, above, which cites the referenced revision in its *first line*:

  "r212427 formalized" ...

, 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,

See the commit log above where I've gone out of my way to explain the existing code, and usage of these structures as part of a cross-thread message-passing system:

  "the message-passing pattern by making these argument structures const."

  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".

That's exactly how I *would* have responded after reading your mail, had you the patience to hold on for a few hours after sending your questions before pushing for a reply in this manner.


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.

Huh. The commits I make in the LLVM project are *systematic* about citing related revisions, historic commits, PRs, even referencing cross-module dependency changes to help people using modular LLVM repositories who might forget to svn up.

I'm very much willing to stand by that claim if you dispute it because I believe these logs have as much, or more, importance than the code they describe and I put real time and effort into them.


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.

So, there are lots of things to prioritize and stabilize for 3.5, but my personal opinion (and indeed, my opinion as probably the most active libclang committer/reviewer since I joined the project) is that libclang's threading model isn't one of those. Regardless, such changes shouldn't be lumped into commits related to the ongoing disk IO quality-of-implementation work.

Finally, there's a need to tread carefully to maintain stability in the C API specifically and this isn't the first place to introduce experimental C++11 techniques we're not even using within clang's core at present -- I'm sure Argyrios and the code owners would echo that sentiment.

Alp.



Thanks,
- David

Alp.



On Mon, Jul 7, 2014 at 2:11 PM, David Blaikie <[email protected]> wrote:
On Sun, Jul 6, 2014 at 6:23 PM, Alp Toker <[email protected]> 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
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

--
http://www.nuanti.com
the browser experts


--
http://www.nuanti.com
the browser experts

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to