Add response file support for command line options by timr1126 · Pull Request #121 · microsoft/xlang

timr1126 · 2019-01-11T19:57:32Z

Previous cppwinrt solution used CommandLineToArgvW to parse the response file, however this uses wide characters and is part of a Windows API, hence not cross-platform. Instead, the parsing mimics what crt does to get the command line to argv and argc. The rest of the logic is very similar to the previous implementation.

kennykerr · 2019-01-11T20:01:42Z

@@ -2,6 +2,8 @@



Did you base/reference your implementation on the CRT implementation?

Yeah, Ben sent me the original code.

kennykerr · 2019-01-11T20:04:00Z


 #include "impl/base.h"

+using namespace std::experimental::filesystem;


Using directives in headers are not good design because it affects everyone including your header and can introduce namespace collisions.

kennykerr · 2019-01-11T20:05:56Z


        std::map<std::string_view, std::vector<std::string>> m_options;
+
+		template<typename O, typename L>


Number one coding guideline: be consistent! 😉

In this case, don't mix tabs and spaces.

kennykerr · 2019-01-11T20:07:58Z

+		}
+
+		template <typename Character>
+		static void parse_command_line(


Where did you get this implementation? If it's basically a copy/paste of the CRT code that's fine it just needs to be cleaned up. Consistency.

It's parse_command_line from argv_parsing.cpp, slightly modified. Sounds good.

Scottj1s · 2019-01-11T20:10:16Z

+			else if (svarg[0] == '@')
+			{
+				svarg.remove_prefix(1);
+				extract_respose_file(svarg, options);


Scottj1s · 2019-01-11T20:11:06Z

        std::map<std::string_view, std::vector<std::string>> m_options;
+
+		template<typename O, typename L>
+		void extract_option(std::string const& sarg, O const& options, L &last) 


consistency: L& last

Scottj1s · 2019-01-11T20:13:00Z

+		template<typename O, typename L>
+		void extract_option(std::string const& sarg, O const& options, L &last) 
+		{
+			std::string_view svarg{ sarg };


do you need svarg (or just use sarg directly)?

kennykerr · 2019-01-11T20:17:07Z

        std::map<std::string_view, std::vector<std::string>> m_options;
+
+		template<typename O, typename L>
+		void extract_option(std::string const& sarg, O const& options, L &last) 


Just make the parameter a string_view and avoid the copies.

kennykerr · 2019-01-11T20:17:24Z

+		template<typename O, typename L>
+		void extract_option(std::string const& sarg, O const& options, L &last) 
+		{
+			std::string_view svarg{ sarg };


This is unnecessary.

Scottj1s · 2019-01-11T20:18:17Z

+		}
+
+		template<typename O>
+		void extract_respose_file(std::string_view const& arg, O const& options)


Scottj1s · 2019-01-11T20:20:16Z

Tim, can we get some standalone unit test cases added under xlang/src/test/cpp/test/? Don't need to drive the exe, just the cmd_reader with expected exceptions for failure cases, etc.

kennykerr · 2019-01-11T20:25:30Z

+		void extract_respose_file(std::string_view const& arg, O const& options)
+		{
+			path response_path{ std::string{ arg } };
+			std::wstring extension = response_path.extension();


extension returns a path, not a wstring. This won't compile on Clang.

kennykerr · 2019-01-15T15:47:02Z

-                    m_options[last->name].push_back(std::string{ arg });
-                }
-            }
+			for (C i = 1; i < argc; ++i)


The code still has white space issues all over the place. Find the "show white space" option in your editor and turn it on. Then change your editor settings to use a tab size of 4 and to insert tabs as spaces. Then cut and paste the entire source file and it should reset the white space consistently. Confirm visually.

kennykerr · 2019-01-15T15:49:05Z

-            }
+			for (C i = 1; i < argc; ++i)
+			{
+				extract_option(std::string_view{ std::string{ argv[i] } }, options, last);


All of these conversions should be unnecessary as the extract_option function takes a string_view. You should be able to simply pass the argv[i] as the first argument.

kennykerr · 2019-01-15T15:51:28Z

        std::map<std::string_view, std::vector<std::string>> m_options;
+
+		template<typename O, typename L>
+		void extract_option(std::string_view arg, O const& options, L& last)


Consistency - use const& for string_view as that done elsewhere in this file.

remove_prefix doesn't work with const& set. Should I change other methods to not be const& for consistency?

Oh, I didn't see that the function modifies the parameter. In that case, this is appropriate and you can just leave it as is.

kennykerr · 2019-01-15T16:04:16Z

+			std::experimental::filesystem::path response_path{ std::string{ arg } };
+			std::string extension = response_path.extension().generic_string();
+			std::transform(extension.begin(), extension.end(), extension.begin(), tolower);
+			if (is_directory(response_path) || extension == ".winmd")


@Scottj1s I see you originally added this check. Any particular reason? I'd rather not have code checking for file extensions since it is not particularly reliable. What if a developer accidentally passes a .exe file name? I believe ifstream will also skip over directories, so perhaps we should just drop this check altogether.

kennykerr · 2019-01-15T16:05:44Z

+				auto last{ options.end() };
+				for (auto i = 0; i < argc; i++)
+				{
+					extract_option(std::string_view{ argv[i] }, options, last);


This conversion should also be unnecessary.

kennykerr · 2019-01-15T16:06:40Z

+			*argument_count  = 0;
+
+			Character c;
+			std::string arg("");


This constructor argument is unnecessary.

kennykerr · 2019-01-15T16:07:39Z

+			Character c;
+			std::string arg("");
+			int copy_character; // 1 = copy char to arg
+			unsigned numslash; // num of backslashes seen


This comment is redundant. Perhaps just call the variable backslash_count.

kennykerr · 2019-01-15T16:08:06Z

+		{
+			*argument_count  = 0;
+
+			Character c;


All the locals in this function should be initialized during declaration.

kennykerr · 2019-01-15T16:10:02Z

+			in_quotes = false;
+			first_arg = true;
+
+			// Loop on each argument


Please remove comments that don't make the code substantially more understandable.

kennykerr · 2019-01-15T16:10:44Z

+
+				first_arg = false;
+			}
+		}


This really needs some unit tests. 😉

Working on em 👍

Scottj1s · 2019-01-16T17:52:53Z

+
+TEST_CASE("CmdReader")
+{
+    const std::vector<option> options


Scottj1s · 2019-01-16T17:53:15Z

+            std::experimental::filesystem::path response_path{ std::string{ arg } };
+            std::string extension = response_path.extension().generic_string();
+            std::transform(extension.begin(), extension.end(), extension.begin(),
+                [](unsigned char c) { return static_cast<unsigned char>(::tolower(c)); });


can make c 'auto'

Scottj1s · 2019-01-16T17:55:50Z

+    {
+        const char* argv[] = { "progname", "@respfile.txt" };
+        const size_t argc = 2;
+        std::ofstream resp_file;


consider making an raii type here to consolidate common logic and delete file in presence of an exception

Might be simpler and more maintainable just to have an actual file in the test project containing the test arguments and not bother with generating and then deleting a file.

I actually like the generation code - everything in one place

Scottj1s · 2019-01-16T17:56:59Z

+    // input and output
+    {
+        const char* argv[] = { "progname", "-in", "example_file.in", "-out", "example_file.out" };
+        const size_t argc = 5;


all argc values can be = _countof(argv)

Scottj1s · 2019-01-16T17:57:43Z

+        std::ofstream resp_file;
+
+        resp_file.open("respfile.txt");
+        resp_file << "-in example\\\"file.in -out example\\\"file.out";


consider raw string literals

Scottj1s · 2019-01-16T18:03:33Z

+            std::string extension = response_path.extension().generic_string();
+            std::transform(extension.begin(), extension.end(), extension.begin(),
+                [](unsigned char c) { return static_cast<unsigned char>(::tolower(c)); });
+            if (is_directory(response_path) || extension == ".winmd")


please add comment that this line is intended to help diagnose misuse of the '@' prefix (e.g., when supplying metadata files)

kennykerr · 2019-01-18T20:00:34Z

Thanks Tim!

Add response file support for command line options

23bb86b

timr1126 requested review from Scottj1s and kennykerr January 11, 2019 19:57

kennykerr reviewed Jan 11, 2019

View reviewed changes

Scottj1s reviewed Jan 11, 2019

View reviewed changes

kennykerr reviewed Jan 11, 2019

View reviewed changes

Scottj1s reviewed Jan 11, 2019

View reviewed changes

kennykerr reviewed Jan 11, 2019

View reviewed changes

kennykerr reviewed Jan 15, 2019

View reviewed changes

Cleanup code

80ba0e7

timr1126 force-pushed the t-tiroma-response-files branch 2 times, most recently from 698314e to 058d2ca Compare January 16, 2019 00:28

Add tests for response files

b8acf63

timr1126 force-pushed the t-tiroma-response-files branch from 058d2ca to b8acf63 Compare January 16, 2019 00:30

Scottj1s reviewed Jan 16, 2019

View reviewed changes

devhawk reviewed Jan 16, 2019

View reviewed changes

Comment thread src/test/cpp/test/CmdReader.cpp

Scottj1s reviewed Jan 16, 2019

View reviewed changes

kennykerr approved these changes Jan 16, 2019

View reviewed changes

timr1126 force-pushed the t-tiroma-response-files branch from f751b80 to 8d2e326 Compare January 16, 2019 23:57

More cleanup

c16ac11

timr1126 force-pushed the t-tiroma-response-files branch from 8d2e326 to c16ac11 Compare January 16, 2019 23:59

Merge branch 'master' into t-tiroma-response-files

49ec12f

timr1126 merged commit 80fac51 into master Jan 18, 2019

timr1126 deleted the t-tiroma-response-files branch January 18, 2019 19:59


		#include "impl/base.h"

		using namespace std::experimental::filesystem;


		std::map<std::string_view, std::vector<std::string>> m_options;

		template<typename O, typename L>

Uh oh!

Conversation

timr1126 commented Jan 11, 2019

Uh oh!

kennykerr Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennykerr Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennykerr Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennykerr Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scottj1s commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timr1126 Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennykerr Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennykerr Jan 11, 2019 •

edited

Loading

kennykerr Jan 11, 2019 •

edited

Loading

kennykerr Jan 11, 2019 •

edited

Loading

kennykerr Jan 11, 2019 •

edited

Loading

timr1126 Jan 15, 2019 •

edited

Loading

kennykerr Jan 15, 2019 •

edited

Loading