[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Nick Desaulniers via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8cce7af090bd: [SourceManager] don't check invalid param 
of getLocalSLocEntry() (authored by nickdesaulniers).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82498/new/

https://reviews.llvm.org/D82498

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1694,11 +1691,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1645,8 +1645,7 @@
   unsigned local_sloc_entry_size() const { return LocalSLocEntryTable.size(); }
 
   /// Get a local SLocEntry. This is exposed for indexing.
-  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
- bool *Invalid = nullptr) const {
+  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");
 return LocalSLocEntryTable[Index];
   }
@@ -1739,12 +1738,13 @@
   const SrcMgr::SLocEntry &loadSLocEntry(unsigned Index, bool *Invalid) const;
 
   /// Get the entry with the given unwrapped FileID.
+  /// Invalid will not be modified for Local IDs.
   const SrcMgr::SLocEntry &getSLocEntryByID(int ID,
 bool *Invalid = nullptr) const {
 assert(ID != -1 && "Using FileID sentinel value");
 if (ID < 0)
   return getLoadedSLocEntryByID(ID, Invalid);
-return getLocalSLocEntry(static_cast(ID), Invalid);
+return getLocalSLocEntry(static_cast(ID));
   }
 
   const SrcMgr::SLocEntry &


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1694,11 +1691,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1645,8 +1645,7 @@
   unsigned local_sloc_entry_size() const { return LocalSLocEntryTable.size(); }
 
   /// Get a local SLocEntry. This is exposed for indexing.
-  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
- bool *Invalid = nullptr) const {
+  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");
 return LocalSLocEntryTable[Index];
   }
@@ -1739,12 +1738,13 @@
   const SrcMgr::SLocEntry &loadSLocEntry(unsigned Index, bool *Invalid) const;
 
   /// Get the entry with the given unwrapped FileID.
+  /// Invalid will not be modified for Local IDs.
   const SrcMgr::SLocEntry &getSLocEntryByID(int ID,

[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82498/new/

https://reviews.llvm.org/D82498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 273449.
nickdesaulniers added a comment.

- drop `Invalid` param from getLocalSLocEntry


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82498/new/

https://reviews.llvm.org/D82498

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1694,11 +1691,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1645,8 +1645,7 @@
   unsigned local_sloc_entry_size() const { return LocalSLocEntryTable.size(); }
 
   /// Get a local SLocEntry. This is exposed for indexing.
-  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
- bool *Invalid = nullptr) const {
+  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");
 return LocalSLocEntryTable[Index];
   }
@@ -1739,12 +1738,13 @@
   const SrcMgr::SLocEntry &loadSLocEntry(unsigned Index, bool *Invalid) const;
 
   /// Get the entry with the given unwrapped FileID.
+  /// Invalid will not be modified for Local IDs.
   const SrcMgr::SLocEntry &getSLocEntryByID(int ID,
 bool *Invalid = nullptr) const {
 assert(ID != -1 && "Using FileID sentinel value");
 if (ID < 0)
   return getLoadedSLocEntryByID(ID, Invalid);
-return getLocalSLocEntry(static_cast(ID), Invalid);
+return getLocalSLocEntry(static_cast(ID));
   }
 
   const SrcMgr::SLocEntry &


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1694,11 +1691,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1645,8 +1645,7 @@
   unsigned local_sloc_entry_size() const { return LocalSLocEntryTable.size(); }
 
   /// Get a local SLocEntry. This is exposed for indexing.
-  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
- bool *Invalid = nullptr) const {
+  const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");
 return LocalSLocEntryTable[Index];
   }
@@ -1739,12 +1738,13 @@
   const SrcMgr::SLocEntry &loadSLocEntry(unsigned Index, bool *Invalid) const;
 
   /// Get the entry with the given unwrapped FileID.
+  /// Invalid will not be modified for Local IDs.
   const SrcMgr::SLocEntry &getSLocEntryByID(int ID,
 bool *Invalid = nullptr) const {
 assert(ID != -1 && "Us

[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:1649
   const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
  bool *Invalid = nullptr) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");

kadircet wrote:
> i think we should drop Invalid from the signature completely if we are going 
> for this.
> 
> I've sent out D82532 to get rid of the libclang dependency on this, after it 
> lands this should be possible to clear.
landed that patch please rebase and get rid of this parameter completely. Also 
please add a comment to `getSLocEntryByID` saying that `Invalid won't be 
modified for local sloc entries`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82498/new/

https://reviews.llvm.org/D82498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/include/clang/Basic/SourceManager.h:1649
   const SrcMgr::SLocEntry &getLocalSLocEntry(unsigned Index,
  bool *Invalid = nullptr) const {
 assert(Index < LocalSLocEntryTable.size() && "Invalid index");

i think we should drop Invalid from the signature completely if we are going 
for this.

I've sent out D82532 to get rid of the libclang dependency on this, after it 
lands this should be possible to clear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82498/new/

https://reviews.llvm.org/D82498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82498: [SourceManager] don't check invalid param of getLocalSLocEntry()

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added a reviewer: kadircet.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
nickdesaulniers retitled this revision from "[SourceManager] don't check 
Invalid param of getLocalSLocEntry()" to "[SourceManager] don't check invalid 
param of getLocalSLocEntry()".

Forked from D80681 .

getLocalSLocEntry() has an unused parameter used to satisfy an interface
of libclang (see getInclusions() in
clang/tools/libclang/CIndexInclusionStack.cpp).  It's pointless for
callers to construct/pass/check this inout parameter that can never
signify that a FileID is invalid.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82498

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1695,11 +1692,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1744,7 +1744,7 @@
 assert(ID != -1 && "Using FileID sentinel value");
 if (ID < 0)
   return getLoadedSLocEntryByID(ID, Invalid);
-return getLocalSLocEntry(static_cast(ID), Invalid);
+return getLocalSLocEntry(static_cast(ID));
   }
 
   const SrcMgr::SLocEntry &


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -882,11 +882,8 @@
   unsigned LessIndex = 0;
   NumProbes = 0;
   while (true) {
-bool Invalid = false;
 unsigned MiddleIndex = (GreaterIndex-LessIndex)/2+LessIndex;
-unsigned MidOffset = getLocalSLocEntry(MiddleIndex, &Invalid).getOffset();
-if (Invalid)
-  return FileID::get(0);
+unsigned MidOffset = getLocalSLocEntry(MiddleIndex).getOffset();
 
 ++NumProbes;
 
@@ -1695,11 +1692,7 @@
   // The location we're looking for isn't in the main file; look
   // through all of the local source locations.
   for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-bool Invalid = false;
-const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-if (Invalid)
-  return FileID();
-
+const SLocEntry &SLoc = getLocalSLocEntry(I);
 if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
 SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
   return FileID::get(I);
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1744,7 +1744,7 @@
 assert(ID != -1 && "Using FileID sentinel value");
 if (ID < 0)
   return getLoadedSLocEntryByID(ID, Invalid);
-return getLocalSLocEntry(static_cast(ID), Invalid);
+return getLocalSLocEntry(static_cast(ID));
   }
 
   const SrcMgr::SLocEntry &
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits