On 16/07/2014 20:59, David Blaikie wrote:
On Thu, Jul 10, 2014 at 7:05 AM, Alp Toker <[email protected]> wrote:
On 08/07/2014 00:11, David Blaikie 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)

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