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

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

Reply via email to