Add support for GetIncludePaths #178

Krishna-13-cyber · 2024-01-04T20:32:09Z

This PR adds GetIncludePaths() to fetch the include paths

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

lib/Interpreter/CppInterOpInterpreter.h

lib/Interpreter/Paths.cpp

codecov · 2024-01-04T20:43:33Z

Codecov Report

Attention: Patch coverage is 43.47826% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (0e75241) to head (72adf22).
Report is 195 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/Interpreter/Paths.cpp	13.33%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   77.55%   77.18%   -0.37%     
==========================================
  Files           8        8              
  Lines        2887     3042     +155     
==========================================
+ Hits         2239     2348     +109     
- Misses        648      694      +46

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`100.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`85.31% <100.00%> (-0.14%)`	⬇️
lib/Interpreter/CppInterOpInterpreter.h	`91.16% <100.00%> (-1.62%)`	⬇️
lib/Interpreter/Paths.cpp	`31.61% <13.33%> (-2.65%)`	⬇️

... and 4 files with indirect coverage changes

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`100.00% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`85.31% <100.00%> (-0.14%)`	⬇️
lib/Interpreter/CppInterOpInterpreter.h	`91.16% <100.00%> (-1.62%)`	⬇️
lib/Interpreter/Paths.cpp	`31.61% <13.33%> (-2.65%)`	⬇️

... and 4 files with indirect coverage changes

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOpInterpreter.h

lib/Interpreter/Paths.cpp

unittests/CppInterOp/InterpreterTest.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2024-01-29T14:03:19Z

lib/Interpreter/Paths.cpp

+    Paths.push_back(PathStr);
+
+  // Avoid duplicates
+  llvm::SmallVector<llvm::StringRef, val> PathsChecked;


warning: unused variable 'Path' [clang-diagnostic-unused-variable]

for (llvm::StringRef Path : PathsChecked) ^

github-actions

clang-tidy made some suggestions

lib/Interpreter/Paths.cpp

vgvassilev · 2024-02-04T08:21:07Z

include/clang/Interpreter/CppInterOp.h

@@ -444,6 +444,9 @@ namespace Cpp {
  /// Returns the resource-dir path (for headers).
  const char* GetResourceDir();

+  /// Get Include Paths
+  void GetIncludePath(const char* dir, std::vector<std::string>& includePaths);


Suggested change

void GetIncludePath(const char* dir, std::vector<std::string>& includePaths);

void GetIncludePath(std::vector<std::string>& includePaths);

This function should only have a single output parameter.

Oh okay, I will make the required changes.

github-actions

clang-tidy made some suggestions

github-actions · 2024-02-07T11:00:04Z

lib/Interpreter/CppInterOpInterpreter.h

@@ -347,6 +347,10 @@ class Interpreter {
        const_cast<const Interpreter*>(this)->getDynamicLibraryManager());
  }

+  /// @brief As a interface to store paths added in AddIncludePaths
+  std::vector<std::string> include;


warning: member variable 'include' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]

std::vector<std::string> include; ^

github-actions · 2024-02-07T11:00:04Z

lib/Interpreter/Paths.cpp

@@ -449,5 +448,32 @@
 #undef  DEBUG_TYPE
 }

+void GetIncludePaths(


warning: an assignment within an 'if' condition is bug-prone [bugprone-assignment-in-if-condition]

if ((Exists = E.Path == Path)) ^

Additional context

lib/Interpreter/Paths.cpp:450: if it should be an assignment, move it out of the 'if' condition

if ((Exists = E.Path == Path)) ^

lib/Interpreter/Paths.cpp:450: if it is meant to be an equality check, change '=' to '=='

if ((Exists = E.Path == Path)) ^

github-actions · 2024-02-07T11:00:04Z

lib/Interpreter/Paths.cpp

+  else
+    Paths.push_back(PathStr);
+
+  // Avoid duplicates


warning: unused variable 'Path' [clang-diagnostic-unused-variable]

for (llvm::StringRef Path : PathsChecked) ^

Krishna-13-cyber · 2024-02-28T15:20:36Z

Hi @vgvassilev,
Pinging you for a review on this updated version, so that we can complete this.

vgvassilev · 2024-02-29T18:05:01Z

lib/Interpreter/CppInterOpInterpreter.h

@@ -347,6 +347,10 @@ class Interpreter {
        const_cast<const Interpreter*>(this)->getDynamicLibraryManager());
  }

+  /// @brief As a interface to store paths added in AddIncludePaths
+  std::vector<std::string> include;


Why do we need this. The include paths are stored in the HeaderSearch already.

So we know that AddincludePaths() is fetching the paths and giving it to HeaderSearch, now GetIncludePaths() had the aim of exposing all the paths which were added(not present in HeaderSearch) to the user, for this we had to get/store all those paths which were being added to HeaderSearch with a vector ( as an interface) as AddIncludePaths() does'nt store any paths (makes it available for HeaderSearch).

Initially, I tried other methods to store all the paths from AddIncludePaths() itself but for that we require a change in function definition and would not require another function GetIncludePaths(). I also tried creating wrappers around AddIncludePaths() but to maintain the function signature I could'nt find a way to store the paths.

github-actions · 2024-12-06T02:43:57Z

This PR is stale because it has been open for 90 days with no activity.

Vipul-Cariappa · 2024-12-18T05:29:59Z

Closed through #311.
Feel free to reopen if I have missed something.

Krishna-13-cyber changed the title ~~Add GetIncludePaths() to fetch the include paths~~ Add support for GetIncludePaths Jan 4, 2024

github-actions bot reviewed Jan 4, 2024

View reviewed changes

github-actions bot reviewed Jan 5, 2024

View reviewed changes

lib/Interpreter/CppInterOpInterpreter.h Outdated Show resolved Hide resolved

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

unittests/CppInterOp/InterpreterTest.cpp Outdated Show resolved Hide resolved

Krishna-13-cyber force-pushed the GetIncludePaths branch from 6819ae4 to e29e778 Compare January 5, 2024 05:14

github-actions bot reviewed Jan 29, 2024

View reviewed changes

Add GetIncludePaths() to fetch the include paths

30b74b1

Krishna-13-cyber force-pushed the GetIncludePaths branch from 19f6118 to 30b74b1 Compare January 29, 2024 19:10

Update with clang-tidy suggestions

e00d0b6

github-actions bot reviewed Feb 1, 2024

View reviewed changes

lib/Interpreter/Paths.cpp Show resolved Hide resolved

vgvassilev reviewed Feb 4, 2024

View reviewed changes

Update with standard signature convention

72adf22

github-actions bot reviewed Feb 7, 2024

View reviewed changes

vgvassilev reviewed Feb 29, 2024

View reviewed changes

aaronj0 added the API The CppInterOp API label May 17, 2024

github-actions bot added the stale label Dec 6, 2024

Vipul-Cariappa closed this Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GetIncludePaths #178

Add support for GetIncludePaths #178

Krishna-13-cyber commented Jan 4, 2024

github-actions bot left a comment

codecov bot commented Jan 4, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot Jan 29, 2024

github-actions bot left a comment

vgvassilev Feb 4, 2024

Krishna-13-cyber Feb 5, 2024

github-actions bot left a comment

github-actions bot Feb 7, 2024

github-actions bot Feb 7, 2024

github-actions bot Feb 7, 2024

Krishna-13-cyber commented Feb 28, 2024

vgvassilev Feb 29, 2024

Krishna-13-cyber Mar 1, 2024

github-actions bot commented Dec 6, 2024

Vipul-Cariappa commented Dec 18, 2024

	void GetIncludePath(const char* dir, std::vector<std::string>& includePaths);
	void GetIncludePath(std::vector<std::string>& includePaths);

Add support for GetIncludePaths #178

Add support for GetIncludePaths #178

Conversation

Krishna-13-cyber commented Jan 4, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 29, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev Feb 4, 2024

Choose a reason for hiding this comment

Krishna-13-cyber Feb 5, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Feb 7, 2024

Choose a reason for hiding this comment

github-actions bot Feb 7, 2024

Choose a reason for hiding this comment

github-actions bot Feb 7, 2024

Choose a reason for hiding this comment

Krishna-13-cyber commented Feb 28, 2024

vgvassilev Feb 29, 2024

Choose a reason for hiding this comment

Krishna-13-cyber Mar 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2024

Vipul-Cariappa commented Dec 18, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading