config_parser: Introduce stricter syntax conventions (#1377)

This is the next step to merge #1237 in stages.

Currently there are barely any restrictions on how the config can be
written. This causes things like config files with DOS line endings to
not be parsed properly (#1366) because polybar splits by `\n` and when
parsing section headers, it can't deal with the `\r` at the end of the
line and thus doesn't recognize any section headers.

With this PR we introduce some rules as to what characters are allowed
in section names and keys.
Note: When talking about spaces I refer to any character for which
`isspace()` returns `true`.

The rules are as follows:
* A section name or a key name cannot contain any spaces as well as any
of there characters:`"'=;#[](){}:.$\%`
* Spaces at the beginning and end of lines are always ignored when
parsing
* Comment lines start with `;` or `#` and last for the whole line. The
whole line will be ignored by the parser. You cannot start a comment at
the end of a line.
* Section headers have the following form `[HEADER_NAME]`
* Key-value lines look like this:
`KEY_NAME{SPACES}={SPACES}VALUE_STRING` where `{SPACES}` represents any
number of spaces. `VALUE_STRING` can contain any characters. If it is
*surrounded* with double quotes (`"`), those quotes will be removed,
this can be used to add spaces to the beginning or end of the value
* Empty lines are lines with only spaces in them
* If the line has any other form, it is a syntax error

This will introduce the following breaking changes because of how
underdefined the config syntax was before:
* `key = ""` will get treated as an empty string instead of the literal
* string `""`
* Any section or key name with forbidden characters will now be syntax
errors.
* Certain strings will be forbidden as section names: `self`, `root`,
* `BAR`. Because they have a special meaning inside references and so a
* section `[root]` can never be referenced.

This replaces the current parser implementation with a new more robust
one that will later be expanded to also check for dependency cycles and
allow for values that contain references mixed with other strings.

This PR also now expands the config paths given over the command line so
that `--config=~/.config/polybar/config` resolves properly.

Closes #1032
Closes #1694

* config_parser: Add skeleton with tests

First step in the config_parser develoment. Only tests functions that
are easily testable without many outside dependencies. Integration tests
will follow.

* config_parser: Implement parse_header

* config_parser: Implement get_line_type

* feat(string): Add trim functions with predicate

Not only trimming based on single character matching but based on a
freely specifiable predicate. Will be used to trim all spaces (based on
isspace)

* config_parser: Implement parse_key

* config_parser: Implement parse_line for valid lines

* config_parser: Throw exception on invalid lines

* config_parser: Remove line_no and file_index from parse_line

Cleaner to let the caller catch and fill in the line number and file
path

* string: Clear up misleading description of trim

Before, trim would remove all characters that *didn't* match the
predicate and thus the predicate isspace wouldn't work correctly. But
because we used the inverse (isnospace_pred) it all worked out, but if
the function was used with any other function, it wouldn't have given
the desired output

* config_parser: Implement parse_file

* config_parser: Switch operation to config_parser

This changes the way the config is invoked. Now main.cpp creates a
config_parser object which then returns the singleton config object from
the parse method. Subsequent calls to config::make will return the
already created config object as before

The config_parser does not yet have all the functionality of the old
parser: `inherit` directives are not yet resolved. Other than that all
the old functionality is implemented (creating sectionmap and applying
include-file)

Any sort of dependency detection (except for include-file) are still
missing

* config: Move xrm initialization to constructor

config_parser handles the detection of xrdb references and passes that
info to the config object.

This finally allows us to delete the config::parse_file function because
everything in it has been implemented (except for xrdb detection and
file error handling)

* refactor(config_parser): Cleanup

* config_parser: Set config data after initialization

Looks much cleaner this way

* config_parser: Expand include-file paths

* config_parser: Init xrm if the config uses %{xrdb references

* config_parser: Use same type of maps as in old impl

Polybar has some weird, not yet fixed, inheriting behaviour and it
changes depending on the order in which the config stores its data.
Using the same type of maps ensures that the behaviour stays the same.

* refactor(config_parser): Clearer invalid name error message

* config_parser: Don't allow reserved section names

Sections with the names 'self', 'BAR', 'root' could never be referenced
because those strings have a special meaning inside references

* config_parser: Handle inherit directives

This uses the old copy_inherited function, so this still suffers from
crashes if there are cyclic dependencies.
This also fixes the behaviour where any key that starts with 'inherit'
would be treated as an inherit directive

* config_parser: Clearer dependency cycle error message

* refactor(config_parser): Handle file errors when parsing

This removes the need to check if the file exists separately

* fix(config): expand config file path

Now paths using ~ and environment variables can be used as the config
path

* fix(config): Properly recognize xrdb references

* config_parser: Make messages more informative

* doc(config): Improve commenting

Comments now describe what the config_parser actually does instead of
what it will do.

We also now follow the rule that single line comments inside functions
should use `//` comments

* refactor: Move else on same line as curly braces

* fix(config_parser): Don't duplicate paths in `files`

* refactor(config_parser): Use else if for clarity

* fix(config): Undefined behavior in syntax_error

Before the custom what() method produced undefined behavior because the
returned string became invalid once the function returned.

* refactor(config): descriptive name for useless lines

is_valid could easily be confused as meaning syntactically invalid
without it being clarified in a comment

* refactor(config): Use separate strings instead of key_value

Takes just as much space and is much better to read

* fix(config_parser): TestCase -> TestSuite and fix macro call

Ref: #1644

* config_parser: use const string& in method args

* config_parser: Improve comments

* config_parser: Incorporate review comments

include/components/config.hpp

@@ -18,19 +18,30 @@ POLYBAR_NS
 DEFINE_ERROR(value_error);
 DEFINE_ERROR(key_error);
 
+using valuemap_t = std::unordered_map<string, string>;
+using sectionmap_t = std::map<string, valuemap_t>;
+using file_list = vector<string>;
+
 class config {
  public:
-  using valuemap_t = std::unordered_map<string, string>;
-  using sectionmap_t = std::map<string, valuemap_t>;
-
   using make_type = const config&;
   static make_type make(string path = "", string bar = "");
 
-  explicit config(const logger& logger, string&& path = "", string&& bar = "");
+  explicit config(const logger& logger, string&& path = "", string&& bar = "")
+      : m_log(logger), m_file(move(path)), m_barname(move(bar)){};
 
-  string filepath() const;
+  const string& filepath() const;
   string section() const;
 
+  /**
+   * \brief Instruct the config to connect to the xresource manager
+   */
+  void use_xrm();
+
+  void set_sections(sectionmap_t sections);
+
+  void set_included(file_list included);
+
   void warn_deprecated(const string& section, const string& key, string replacement) const;
 
   /**
@@ -193,7 +204,6 @@ class config {
   }
 
  protected:
-  void parse_file();
   void copy_inherited();
 
   template <typename T>
@@ -366,6 +376,12 @@ class config {
   string m_file;
   string m_barname;
   sectionmap_t m_sections{};
+
+  /**
+   * Absolute path of all files that were parsed in the process of parsing the
+   * config (Path of the main config file also included)
+   */
+  file_list m_included;
 #if WITH_XRM
   unique_ptr<xresource_manager> m_xrm;
 #endif

include/components/config_parser.hpp

@@ -0,0 +1,249 @@
+#pragma once
+
+#include <set>
+
+#include "common.hpp"
+#include "components/config.hpp"
+#include "components/logger.hpp"
+#include "errors.hpp"
+#include "utils/file.hpp"
+#include "utils/string.hpp"
+
+POLYBAR_NS
+
+DEFINE_ERROR(parser_error);
+
+/**
+ * \brief Exception object for syntax errors
+ *
+ * Contains filepath and line number where syntax error was found
+ */
+class syntax_error : public parser_error {
+ public:
+  /**
+   * Default values are used when the thrower doesn't know the position.
+   * parse_line has to catch, set the proper values and rethrow
+   */
+  explicit syntax_error(string msg, const string& file = "", int line_no = -1)
+      : parser_error(file + ":" + to_string(line_no) + ": " + msg), msg(move(msg)) {}
+
+  const string& get_msg() {
+    return msg;
+  };
+
+ private:
+  string msg;
+};
+
+class invalid_name_error : public syntax_error {
+ public:
+  /**
+   * type is either Header or Key
+   */
+  invalid_name_error(const string& type, const string& name)
+      : syntax_error(type + " '" + name + "' contains forbidden characters.") {}
+};
+
+/**
+ * \brief All different types a line in a config can be
+ */
+enum class line_type { KEY, HEADER, COMMENT, EMPTY, UNKNOWN };
+
+/**
+ * \brief Storage for a single config line
+ *
+ * More sanitized than the actual string of the comment line, with information
+ * about line type and structure
+ */
+struct line_t {
+  /**
+   * Whether or not this struct represents a "useful" line, a line that has
+   * any semantic significance (key-value or header line)
+   * If false all other fields are not set.
+   * Set this to false, if you want to return a line that has no effect
+   * (for example when you parse a comment line)
+   */
+  bool useful;
+
+  /**
+   * Index of the config_parser::files vector where this line is from
+   */
+  int file_index;
+  int line_no;
+
+  /**
+   * We access header, if is_header == true otherwise we access key, value
+   */
+  bool is_header;
+
+  /**
+   * Only set for header lines
+   */
+  string header;
+
+  /**
+   * Only set for key-value lines
+   */
+  string key, value;
+};
+
+class config_parser {
+ public:
+  config_parser(const logger& logger, string&& file, string&& bar);
+
+  /**
+   * \brief Performs the parsing of the main config file m_file
+   *
+   * \returns config class instance populated with the parsed config
+   *
+   * \throws syntax_error If there was any kind of syntax error
+   * \throws parser_error If aynthing else went wrong
+   */
+  config::make_type parse();
+
+ protected:
+  /**
+   * \brief Converts the `lines` vector to a proper sectionmap
+   */
+  sectionmap_t create_sectionmap();
+
+  /**
+   * \brief Parses the given file, extracts key-value pairs and section
+   *        headers and adds them onto the `lines` vector
+   *
+   * This method directly resolves `include-file` directives and checks for
+   * cyclic dependencies
+   *
+   * `file` is expected to be an already resolved absolute path
+   */
+  void parse_file(const string& file, file_list path);
+
+  /**
+   * \brief Parses the given line string to create a line_t struct
+   *
+   * We use the INI file syntax (https://en.wikipedia.org/wiki/INI_file)
+   * Whitespaces (tested with isspace()) at the beginning and end of a line are ignored
+   * Keys and section names can contain any character except for the following:
+   * - spaces
+   * - equal sign (=)
+   * - semicolon (;)
+   * - pound sign (#)
+   * - Any kind of parentheses ([](){})
+   * - colon (:)
+   * - period (.)
+   * - dollar sign ($)
+   * - backslash (\)
+   * - percent sign (%)
+   * - single and double quotes ('")
+   * So basically any character that has any kind of special meaning is prohibited.
+   *
+   * Comment lines have to start with a semicolon (;) or a pound sign (#),
+   * you cannot put a comment after another type of line.
+   *
+   * key and section names are case-sensitive.
+   *
+   * Keys are specified as `key = value`, spaces around the equal sign, as
+   * well as double quotes around the value are ignored
+   *
+   * sections are defined as [section], everything inside the square brackets is part of the name
+   *
+   * \throws syntax_error if the line isn't well formed. The syntax error
+   *         does not contain the filename or line numbers because parse_line
+   *         doesn't know about those. Whoever calls parse_line needs to
+   *         catch those exceptions and set the file path and line number
+   */
+  line_t parse_line(const string& line);
+
+  /**
+   * \brief Determines the type of a line read from a config file
+   *
+   * Expects that line is trimmed
+   * This mainly looks at the first character and doesn't check if the line is
+   * actually syntactically correct.
+   * HEADER ('['), COMMENT (';' or '#') and EMPTY (None) are uniquely
+   * identified by their first character (or lack thereof). Any line that
+   * is none of the above and contains an equal sign, is treated as KEY.
+   * All others are UNKNOWN
+   */
+  static line_type get_line_type(const string& line);
+
+  /**
+   * \brief Parse a line containing a section header and returns the header name
+   *
+   * Only assumes that the line starts with '[' and is trimmed
+   *
+   * \throws syntax_error if the line doesn't end with ']' or the header name
+   *         contains forbidden characters
+   */
+  string parse_header(const string& line);
+
+  /**
+   * \brief Parses a line containing a key-value pair and returns the key name
+   *        and the value string inside an std::pair
+   *
+   * Only assumes that the line contains '=' at least once and is trimmed
+   *
+   * \throws syntax_error if the key contains forbidden characters
+   */
+  std::pair<string, string> parse_key(const string& line);
+
+  /**
+   * \brief Name of all the files the config includes values from
+   *
+   * The line_t struct uses indices to this vector to map lines to their
+   * original files. This allows us to point the user to the exact location
+   * of errors
+   */
+  file_list m_files;
+
+ private:
+  /**
+   * \brief Checks if the given name doesn't contain any spaces or characters
+   *        in config_parser::m_forbidden_chars
+   */
+  bool is_valid_name(const string& name);
+
+  /**
+   * \brief Whether or not an xresource manager should be used
+   *
+   * Is set to true if any ${xrdb...} references are found
+   */
+  bool use_xrm{false};
+
+  const logger& m_log;
+
+  /**
+   * \brief Absolute path to the main config file
+   */
+  string m_config;
+
+  /**
+   * Is used to resolve ${root...} references
+   */
+  string m_barname;
+
+  /**
+   * \brief List of all the lines in the config (with included files)
+   *
+   * The order here matters, as we have not yet associated key-value pairs
+   * with sections
+   */
+  vector<line_t> m_lines;
+
+  /**
+   * \brief None of these characters can be used in the key and section names
+   */
+  const string m_forbidden_chars{"\"'=;#[](){}:.$\\%"};
+
+  /**
+   * \brief List of names that cannot be used as section names
+   *
+   * These strings have a special meaning inside references and so the
+   * section [self] could never be referenced.
+   *
+   * Note: BAR is deprecated
+   */
+  const std::set<string> m_reserved_section_names = {"self", "BAR", "root"};
+};
+
+POLYBAR_NS_END

include/utils/string.hpp

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <sstream>
-#include <cstring>
 
 #include "common.hpp"
 
@@ -27,7 +26,7 @@ namespace {
       a.erase(a.size() - b.size());
     }
   }
-}
+}  // namespace
 
 class sstream {
  public:
@@ -77,6 +76,10 @@ namespace string_util {
   string strip(const string& haystack, char needle);
   string strip_trailing_newline(const string& haystack);
 
+  string ltrim(string value, function<bool(char)> pred);
+  string rtrim(string value, function<bool(char)> pred);
+  string trim(string value, function<bool(char)> pred);
+
   string ltrim(string&& value, const char& needle = ' ');
   string rtrim(string&& value, const char& needle = ' ');
   string trim(string&& value, const char& needle = ' ');
@@ -96,6 +99,6 @@ namespace string_util {
   string filesize(unsigned long long kbytes, size_t precision = 0, bool fixed = false, const string& locale = "");
 
   hash_type hash(const string& src);
-}
+}  // namespace string_util
 
 POLYBAR_NS_END

src/components/config.cpp

@@ -6,8 +6,6 @@
 #include "utils/color.hpp"
 #include "utils/env.hpp"
 #include "utils/factory.hpp"
-#include "utils/file.hpp"
-#include "utils/math.hpp"
 #include "utils/string.hpp"
 
 POLYBAR_NS
@@ -19,39 +17,10 @@ config::make_type config::make(string path, string bar) {
   return *factory_util::singleton<std::remove_reference_t<config::make_type>>(logger::make(), move(path), move(bar));
 }
 
-/**
- * Construct config object
- */
-config::config(const logger& logger, string&& path, string&& bar)
-    : m_log(logger), m_file(forward<string>(path)), m_barname(forward<string>(bar)) {
-  if (!file_util::exists(m_file)) {
-    throw application_error("Could not find config file: " + m_file);
-  }
-
-  m_log.info("Loading config: %s", m_file);
-
-  parse_file();
-  copy_inherited();
-
-  bool found_bar{false};
-  for (auto&& p : m_sections) {
-    if (p.first == section()) {
-      found_bar = true;
-      break;
-    }
-  }
-
-  if (!found_bar) {
-    throw application_error("Undefined bar: " + m_barname);
-  }
-
-  m_log.trace("config: Current bar section: [%s]", section());
-}
-
 /**
  * Get path of loaded file
  */
-string config::filepath() const {
+const string& config::filepath() const {
   return m_file;
 }
 
@@ -62,6 +31,28 @@ string config::section() const {
   return "bar/" + m_barname;
 }
 
+void config::use_xrm() {
+#if WITH_XRM
+  /*
+   * Initialize the xresource manager if there are any xrdb refs
+   * present in the configuration
+   */
+  if (!m_xrm) {
+    m_log.info("Enabling xresource manager");
+    m_xrm.reset(new xresource_manager{connection::make()});
+  }
+#endif
+}
+
+void config::set_sections(sectionmap_t sections) {
+  m_sections = move(sections);
+  copy_inherited();
+}
+
+void config::set_included(file_list included) {
+  m_included = move(included);
+}
+
 /**
  * Print a deprecation warning if the given parameter is set
  */
@@ -74,117 +65,17 @@ void config::warn_deprecated(const string& section, const string& key, string re
   }
 }
 
-/**
- * Parse key/value pairs from the configuration file
- */
-void config::parse_file() {
-  vector<pair<int, string>> lines;
-  vector<string> files{m_file};
-
-  std::function<void(int, string&&)> pushline = [&](int lineno, string&& line) {
-    // Ignore empty lines and comments
-    if (line.empty() || line[0] == ';' || line[0] == '#') {
-      return;
-    }
-
-    string key, value;
-    string::size_type pos;
-
-    // Filter lines by:
-    // - key/value pairs
-    // - section headers
-    if ((pos = line.find('=')) != string::npos) {
-      key = forward<string>(string_util::trim(forward<string>(line.substr(0, pos))));
-      value = forward<string>(string_util::trim(line.substr(pos + 1)));
-    } else if (line[0] != '[' || line[line.length() - 1] != ']') {
-      return;
-    }
-
-    if (key == "include-file") {
-      auto file_path = file_util::expand(value);
-      if (file_path.empty() || !file_util::exists(file_path)) {
-        throw value_error("Invalid include file \"" + file_path + "\" defined on line " + to_string(lineno));
-      }
-      if (std::find(files.begin(), files.end(), file_path) != files.end()) {
-        throw value_error("Recursive include file \"" + file_path + "\"");
-      }
-      files.push_back(file_util::expand(file_path));
-      m_log.trace("config: Including file \"%s\"", file_path);
-      for (auto&& l : string_util::split(file_util::contents(file_path), '\n')) {
-        pushline(lineno, forward<string>(l));
-      }
-      files.pop_back();
-    } else {
-      lines.emplace_back(make_pair(lineno, line));
-    }
-  };
-
-  int lineno{0};
-  string line;
-  std::ifstream in(m_file);
-  while (std::getline(in, line)) {
-    pushline(++lineno, string_util::replace_all(line, "\t", ""));
-  }
-
-  string section;
-  for (auto&& l : lines) {
-    auto& lineno = l.first;
-    auto& line = l.second;
-
-    // New section
-    if (line[0] == '[' && line[line.length() - 1] == ']') {
-      section = line.substr(1, line.length() - 2);
-      continue;
-    } else if (section.empty()) {
-      continue;
-    }
-
-    size_t equal_pos;
-
-    // Check for key-value pair equal sign
-    if ((equal_pos = line.find('=')) == string::npos) {
-      continue;
-    }
-
-    string key{forward<string>(string_util::trim(forward<string>(line.substr(0, equal_pos))))};
-    string value;
-
-    auto it = m_sections[section].find(key);
-    if (it != m_sections[section].end()) {
-      throw key_error("Duplicate key name \"" + key + "\" defined on line " + to_string(lineno));
-    }
-
-    if (equal_pos + 1 < line.size()) {
-      value = forward<string>(string_util::trim(line.substr(equal_pos + 1)));
-      size_t len{value.size()};
-      if (len > 2 && value[0] == '"' && value[len - 1] == '"') {
-        value.erase(len - 1, 1).erase(0, 1);
-      }
-    }
-
-#if WITH_XRM
-    // Initialize the xresource manage if there are any xrdb refs
-    // present in the configuration
-    if (!m_xrm && value.find("${xrdb") != string::npos) {
-      m_xrm.reset(new xresource_manager{connection::make()});
-    }
-#endif
-
-    m_sections[section].emplace_hint(it, move(key), move(value));
-  }
-}
-
 /**
  * Look for sections set up to inherit from a base section
  * and copy the missing parameters
  *
- *   [sub/seciton]
+ *   [sub/section]
  *   inherit = base/section
  */
 void config::copy_inherited() {
   for (auto&& section : m_sections) {
     for (auto&& param : section.second) {
-      if (param.first.find("inherit") == 0) {
+      if (param.first == "inherit") {
         // Get name of base section
         auto inherit = param.second;
         if ((inherit = dereference<string>(section.first, param.first, inherit, inherit)).empty()) {
@@ -199,10 +90,12 @@ void config::copy_inherited() {
 
         m_log.trace("config: Copying missing params (sub=\"%s\", base=\"%s\")", section.first, inherit);
 
-        // Iterate the base and copy the parameters
+        /*
-        // that hasn't been defined for the sub-section
+         * Iterate the base and copy the parameters that haven't been defined
+         * for the sub-section
+         */
         for (auto&& base_param : base_section->second) {
-          section.second.insert(make_pair(base_param.first, base_param.second));
+          section.second.emplace(base_param.first, base_param.second);
         }
       }
     }

src/components/config_parser.cpp

@@ -0,0 +1,269 @@
+#include <algorithm>
+#include <fstream>
+
+#include "components/config_parser.hpp"
+
+POLYBAR_NS
+
+config_parser::config_parser(const logger& logger, string&& file, string&& bar)
+    : m_log(logger), m_config(file_util::expand(file)), m_barname(move(bar)) {}
+
+config::make_type config_parser::parse() {
+  m_log.info("Parsing config file: %s", m_config);
+
+  parse_file(m_config, {});
+
+  sectionmap_t sections = create_sectionmap();
+
+  if (sections.find("bar/" + m_barname) == sections.end()) {
+    throw application_error("Undefined bar: " + m_barname);
+  }
+
+  /*
+   * The first element in the files vector is always the main config file and
+   * because it has unique filenames, we can use all the elements from the
+   * second element onwards for the included list
+   */
+  file_list included(m_files.begin() + 1, m_files.end());
+  config::make_type result = config::make(m_config, m_barname);
+
+  // Cast to non-const to set sections, included and xrm
+  config& m_conf = const_cast<config&>(result);
+
+  m_conf.set_sections(move(sections));
+  m_conf.set_included(move(included));
+  if (use_xrm) {
+    m_conf.use_xrm();
+  }
+
+  return result;
+}
+
+sectionmap_t config_parser::create_sectionmap() {
+  sectionmap_t sections{};
+
+  string current_section{};
+
+  for (const line_t& line : m_lines) {
+    if (!line.useful) {
+      continue;
+    }
+
+    if (line.is_header) {
+      current_section = line.header;
+    } else {
+      // The first valid line in the config is not a section definition
+      if (current_section.empty()) {
+        throw syntax_error("First valid line in config must be section header", m_files[line.file_index], line.line_no);
+      }
+
+      const string& key = line.key;
+      const string& value = line.value;
+
+      valuemap_t& valuemap = sections[current_section];
+
+      if (valuemap.find(key) == valuemap.end()) {
+        valuemap.emplace(key, value);
+      } else {
+        // Key already exists in this section
+        throw syntax_error("Duplicate key name \"" + key + "\" defined in section \"" + current_section + "\"",
+            m_files[line.file_index], line.line_no);
+      }
+    }
+  }
+
+  return sections;
+}
+
+void config_parser::parse_file(const string& file, file_list path) {
+  if (std::find(path.begin(), path.end(), file) != path.end()) {
+    string path_str{};
+
+    for (const auto& p : path) {
+      path_str += ">\t" + p + "\n";
+    }
+
+    path_str += ">\t" + file;
+
+    // We have already parsed this file in this path, so there are cyclic dependencies
+    throw application_error("include-file: Dependency cycle detected:\n" + path_str);
+  }
+
+  m_log.trace("config_parser: Parsing %s", file);
+
+  int file_index;
+
+  auto found = std::find(m_files.begin(), m_files.end(), file);
+
+  if (found == m_files.end()) {
+    file_index = m_files.size();
+    m_files.push_back(file);
+  } else {
+    /*
+     * `file` is already in the `files` vector so we calculate its index.
+     *
+     * This means that the file was already parsed, this can happen without
+     * cyclic dependencies, if the file is included twice
+     */
+    file_index = found - m_files.begin();
+  }
+
+  path.push_back(file);
+
+  int line_no = 0;
+
+  string line_str{};
+
+  std::ifstream in(file);
+
+  if (!in) {
+    throw application_error("Failed to open config file " + file + ": " + strerror(errno));
+  }
+
+  while (std::getline(in, line_str)) {
+    line_no++;
+    line_t line;
+    try {
+      line = parse_line(line_str);
+
+      // parse_line doesn't set these
+      line.file_index = file_index;
+      line.line_no = line_no;
+    } catch (syntax_error& err) {
+      /*
+       * Exceptions thrown by parse_line doesn't have the line
+       * numbers and files set, so we have to add them here
+       */
+      throw syntax_error(err.get_msg(), m_files[file_index], line_no);
+    }
+
+    // Skip useless lines (comments, empty lines)
+    if (!line.useful) {
+      continue;
+    }
+
+    if (!line.is_header && line.key == "include-file") {
+      parse_file(file_util::expand(line.value), path);
+    } else {
+      m_lines.push_back(line);
+    }
+  }
+}
+
+line_t config_parser::parse_line(const string& line) {
+  string line_trimmed = string_util::trim(line, isspace);
+  line_type type = get_line_type(line_trimmed);
+
+  line_t result = {};
+
+  if (type == line_type::EMPTY || type == line_type::COMMENT) {
+    result.useful = false;
+    return result;
+  }
+
+  if (type == line_type::UNKNOWN) {
+    throw syntax_error("Unknown line type: " + line_trimmed);
+  }
+
+  result.useful = true;
+
+  if (type == line_type::HEADER) {
+    result.is_header = true;
+    result.header = parse_header(line_trimmed);
+  } else if (type == line_type::KEY) {
+    result.is_header = false;
+    auto key_value = parse_key(line_trimmed);
+    result.key = key_value.first;
+    result.value = key_value.second;
+  }
+
+  return result;
+}
+
+line_type config_parser::get_line_type(const string& line) {
+  if (line.empty()) {
+    return line_type::EMPTY;
+  }
+
+  switch (line[0]) {
+    case '[':
+      return line_type::HEADER;
+
+    case ';':
+    case '#':
+      return line_type::COMMENT;
+
+    default:
+      if (string_util::contains(line, "=")) {
+        return line_type::KEY;
+      } else {
+        return line_type::UNKNOWN;
+      }
+  }
+}
+
+string config_parser::parse_header(const string& line) {
+  if (line.back() != ']') {
+    throw syntax_error("Missing ']' in header '" + line + "'");
+  }
+
+  // Stripping square brackets
+  string header = line.substr(1, line.size() - 2);
+
+  if (!is_valid_name(header)) {
+    throw invalid_name_error("Section", header);
+  }
+
+  if (m_reserved_section_names.find(header) != m_reserved_section_names.end()) {
+    throw syntax_error("'" + header + "' is reserved and cannot be used as a section name");
+  }
+
+  return header;
+}
+
+std::pair<string, string> config_parser::parse_key(const string& line) {
+  size_t pos = line.find_first_of('=');
+
+  string key = string_util::trim(line.substr(0, pos), isspace);
+  string value = string_util::trim(line.substr(pos + 1), isspace);
+
+  if (!is_valid_name(key)) {
+    throw invalid_name_error("Key", key);
+  }
+
+  /*
+   * Only if the string is surrounded with double quotes, do we treat them
+   * not as part of the value and remove them.
+   */
+  if (value.size() >= 2 && value.front() == '"' && value.back() == '"') {
+    value = value.substr(1, value.size() - 2);
+  }
+
+  // TODO check value for references
+
+#if WITH_XRM
+  // Use xrm, if at least one value is an xrdb reference
+  if (!use_xrm && value.find("${xrdb") == 0) {
+    use_xrm = true;
+  }
+#endif
+
+  return {move(key), move(value)};
+}
+
+bool config_parser::is_valid_name(const string& name) {
+  if (name.empty()) {
+    return false;
+  }
+
+  for (const char c : name) {
+    // Names with forbidden chars or spaces are not valid
+    if (isspace(c) || m_forbidden_chars.find_first_of(c) != string::npos) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+POLYBAR_NS_END

src/main.cpp

@@ -1,6 +1,7 @@
 #include "components/bar.hpp"
 #include "components/command_line.hpp"
 #include "components/config.hpp"
+#include "components/config_parser.hpp"
 #include "components/controller.hpp"
 #include "components/ipc.hpp"
 #include "utils/env.hpp"
@@ -81,7 +82,8 @@ int main(int argc, char** argv) {
           printf("%s: %ix%i+%i+%i (XRandR monitor%s)\n", mon->name.c_str(), mon->w, mon->h, mon->x, mon->y,
               mon->primary ? ", primary" : "");
         } else {
-          printf("%s: %ix%i+%i+%i%s\n", mon->name.c_str(), mon->w, mon->h, mon->x, mon->y, mon->primary ? " (primary)" : "");
+          printf("%s: %ix%i+%i+%i%s\n", mon->name.c_str(), mon->w, mon->h, mon->x, mon->y,
+              mon->primary ? " (primary)" : "");
         }
       }
       return EXIT_SUCCESS;
@@ -112,7 +114,8 @@ int main(int argc, char** argv) {
       throw application_error("Define configuration using --config=PATH");
     }
 
-    config::make_type conf{config::make(move(confpath), cli->get(0))};
+    config_parser parser{logger, move(confpath), cli->get(0)};
+    config::make_type conf = parser.parse();
 
     //==================================================
     // Dump requested data

src/utils/string.cpp

@@ -1,5 +1,4 @@
 #include <algorithm>
-#include <cstring>
 #include <iomanip>
 #include <sstream>
 #include <utility>
@@ -110,6 +109,29 @@ namespace string_util {
     return str;
   }
 
+  /**
+   * Trims all characters that match pred from the left
+   */
+  string ltrim(string value, function<bool(char)> pred) {
+    value.erase(value.begin(), find_if(value.begin(), value.end(), not1(pred)));
+    return value;
+  }
+
+  /**
+   * Trims all characters that match pred from the right
+   */
+  string rtrim(string value, function<bool(char)> pred) {
+    value.erase(find_if(value.rbegin(), value.rend(), not1(pred)).base(), value.end());
+    return value;
+  }
+
+  /**
+   * Trims all characters that match pred from both sides
+   */
+  string trim(string value, function<bool(char)> pred) {
+    return ltrim(rtrim(move(value), pred), pred);
+  }
+
   /**
    * Remove needle from the start of the string
    */
@@ -275,6 +297,6 @@ namespace string_util {
   hash_type hash(const string& src) {
     return std::hash<string>()(src);
   }
-}
+}  // namespace string_util
 
 POLYBAR_NS_END

tests/CMakeLists.txt

@@ -57,6 +57,7 @@ add_unit_test(components/command_line)
 add_unit_test(components/bar)
 add_unit_test(components/builder)
 add_unit_test(components/parser)
+add_unit_test(components/config_parser)
 
 # Run make check to build and run all unit tests
 add_custom_target(check

tests/unit_tests/components/config_parser.cpp

@@ -0,0 +1,236 @@
+#include "components/config_parser.hpp"
+#include "common/test.hpp"
+#include "components/logger.hpp"
+
+using namespace polybar;
+using namespace std;
+
+/**
+ * \brief Testing-only subclass of config_parser to change access level
+ */
+class TestableConfigParser : public config_parser {
+  using config_parser::config_parser;
+
+ public:
+  using config_parser::get_line_type;
+
+ public:
+  using config_parser::parse_key;
+
+ public:
+  using config_parser::parse_header;
+
+ public:
+  using config_parser::parse_line;
+
+ public:
+  using config_parser::m_files;
+};
+
+/**
+ * \brief Fixture class
+ */
+class ConfigParser : public ::testing::Test {
+ protected:
+  unique_ptr<TestableConfigParser> parser =
+      make_unique<TestableConfigParser>(logger(loglevel::NONE), "/dev/zero", "TEST");
+};
+
+// ParseLineTest {{{
+class ParseLineInValidTest : public ConfigParser, public ::testing::WithParamInterface<string> {};
+
+class ParseLineHeaderTest : public ConfigParser, public ::testing::WithParamInterface<pair<string, string>> {};
+
+class ParseLineKeyTest : public ConfigParser,
+                         public ::testing::WithParamInterface<pair<pair<string, string>, string>> {};
+
+vector<string> parse_line_invalid_list = {
+    " # comment",
+    "; comment",
+    "\t#",
+    "",
+    " ",
+    "\t ",
+};
+
+vector<pair<string, string>> parse_line_header_list = {
+    {"section", "\t[section]"},
+    {"section", "\t[section]  "},
+    {"bar/name", "\t[bar/name]  "},
+};
+
+vector<pair<pair<string, string>, string>> parse_line_key_list = {
+    {{"key", "value"}, " key = value"},
+    {{"key", ""}, " key\t = \"\""},
+    {{"key", "\""}, " key\t = \"\"\""},
+};
+
+INSTANTIATE_TEST_SUITE_P(Inst, ParseLineInValidTest, ::testing::ValuesIn(parse_line_invalid_list));
+
+TEST_P(ParseLineInValidTest, correctness) {
+  line_t line = parser->parse_line(GetParam());
+
+  EXPECT_FALSE(line.useful);
+}
+
+INSTANTIATE_TEST_SUITE_P(Inst, ParseLineHeaderTest, ::testing::ValuesIn(parse_line_header_list));
+
+TEST_P(ParseLineHeaderTest, correctness) {
+  line_t line = parser->parse_line(GetParam().second);
+
+  EXPECT_TRUE(line.useful);
+
+  EXPECT_TRUE(line.is_header);
+  EXPECT_EQ(GetParam().first, line.header);
+}
+
+INSTANTIATE_TEST_SUITE_P(Inst, ParseLineKeyTest, ::testing::ValuesIn(parse_line_key_list));
+
+TEST_P(ParseLineKeyTest, correctness) {
+  line_t line = parser->parse_line(GetParam().second);
+
+  EXPECT_TRUE(line.useful);
+
+  EXPECT_FALSE(line.is_header);
+  EXPECT_EQ(GetParam().first.first, line.key);
+  EXPECT_EQ(GetParam().first.second, line.value);
+}
+
+TEST_F(ParseLineInValidTest, throwsSyntaxError) {
+  EXPECT_THROW(parser->parse_line("unknown"), syntax_error);
+}
+// }}}
+
+// GetLineTypeTest {{{
+
+/**
+ * \brief Class for parameterized tests on get_line_type
+ *
+ * Parameters are pairs of the expected line type and a string that should be
+ * detected as that line type
+ */
+class GetLineTypeTest : public ConfigParser, public ::testing::WithParamInterface<pair<line_type, string>> {};
+
+/**
+ * \brief Helper function generate GetLineTypeTest parameter values
+ */
+vector<pair<line_type, string>> line_type_transform(vector<string>&& in, line_type type) {
+  vector<pair<line_type, string>> out;
+
+  out.reserve(in.size());
+  for (const auto& i : in) {
+    out.emplace_back(type, i);
+  }
+
+  return out;
+}
+
+/**
+ * \brief Parameter values for GetLineTypeTest
+ */
+auto line_type_key = line_type_transform({"a = b", "  a =b", " a\t =\t \t b", "a = "}, line_type::KEY);
+auto line_type_header = line_type_transform({"[section]", "[section]", "[section/sub]"}, line_type::HEADER);
+auto line_type_comment = line_type_transform({";abc", "#abc", ";", "#"}, line_type::COMMENT);
+auto line_type_empty = line_type_transform({""}, line_type::EMPTY);
+auto line_type_unknown = line_type_transform({"|a", " |a", "a"}, line_type::UNKNOWN);
+
+/**
+ * Instantiate GetLineTypeTest for the different line types
+ */
+INSTANTIATE_TEST_SUITE_P(LineTypeKey, GetLineTypeTest, ::testing::ValuesIn(line_type_key));
+INSTANTIATE_TEST_SUITE_P(LineTypeHeader, GetLineTypeTest, ::testing::ValuesIn(line_type_header));
+INSTANTIATE_TEST_SUITE_P(LineTypeComment, GetLineTypeTest, ::testing::ValuesIn(line_type_comment));
+INSTANTIATE_TEST_SUITE_P(LineTypeEmpty, GetLineTypeTest, ::testing::ValuesIn(line_type_empty));
+INSTANTIATE_TEST_SUITE_P(LineTypeUnknown, GetLineTypeTest, ::testing::ValuesIn(line_type_unknown));
+
+/**
+ * \brief Parameterized test for get_line_type
+ */
+TEST_P(GetLineTypeTest, correctness) {
+  EXPECT_EQ(GetParam().first, parser->get_line_type(GetParam().second));
+}
+
+// }}}
+
+// ParseKeyTest {{{
+
+/**
+ * \brief Class for parameterized tests on parse_key
+ *
+ * The first element of the pair is the expected key-value pair and the second
+ * element is the string to be parsed, has to be trimmed and valid.
+ */
+class ParseKeyTest : public ConfigParser, public ::testing::WithParamInterface<pair<pair<string, string>, string>> {};
+
+vector<pair<pair<string, string>, string>> parse_key_list = {
+    {{"key", "value"}, "key = value"},
+    {{"key", "value"}, "key=value"},
+    {{"key", "value"}, "key =\"value\""},
+    {{"key", "value"}, "key\t=\t \"value\""},
+    {{"key", "\"value"}, "key = \"value"},
+    {{"key", "value\""}, "key = value\""},
+    {{"key", "= value"}, "key == value"},
+    {{"key", ""}, "key ="},
+    {{"key", ""}, R"(key ="")"},
+    {{"key", "\"\""}, R"(key ="""")"},
+};
+
+INSTANTIATE_TEST_SUITE_P(Inst, ParseKeyTest, ::testing::ValuesIn(parse_key_list));
+
+/**
+ * Parameterized test for parse_key with valid line
+ */
+TEST_P(ParseKeyTest, correctness) {
+  EXPECT_EQ(GetParam().first, parser->parse_key(GetParam().second));
+}
+
+/**
+ * Tests if exception is thrown for invalid key line
+ */
+TEST_F(ParseKeyTest, throwsSyntaxError) {
+  EXPECT_THROW(parser->parse_key("= empty name"), syntax_error);
+  EXPECT_THROW(parser->parse_key("forbidden char = value"), syntax_error);
+  EXPECT_THROW(parser->parse_key("forbidden\tchar = value"), syntax_error);
+}
+// }}}
+
+// ParseHeaderTest {{{
+
+/**
+ * \brief Class for parameterized tests on parse_key
+ *
+ * The first element of the pair is the expected key-value pair and the second
+ * element is the string to be parsed, has to be trimmed and valid
+ */
+class ParseHeaderTest : public ConfigParser, public ::testing::WithParamInterface<pair<string, string>> {};
+
+vector<pair<string, string>> parse_header_list = {
+    {"section", "[section]"},
+    {"bar/name", "[bar/name]"},
+    {"with_underscore", "[with_underscore]"},
+};
+
+INSTANTIATE_TEST_SUITE_P(Inst, ParseHeaderTest, ::testing::ValuesIn(parse_header_list));
+
+/**
+ * Parameterized test for parse_header with valid line
+ */
+TEST_P(ParseHeaderTest, correctness) {
+  EXPECT_EQ(GetParam().first, parser->parse_header(GetParam().second));
+}
+
+/**
+ * Tests if exception is thrown for invalid header line
+ */
+TEST_F(ParseHeaderTest, throwsSyntaxError) {
+  EXPECT_THROW(parser->parse_header("[]"), syntax_error);
+  EXPECT_THROW(parser->parse_header("[no_end"), syntax_error);
+  EXPECT_THROW(parser->parse_header("[forbidden char]"), syntax_error);
+  EXPECT_THROW(parser->parse_header("[forbidden\tchar]"), syntax_error);
+
+  // Reserved names
+  EXPECT_THROW(parser->parse_header("[self]"), syntax_error);
+  EXPECT_THROW(parser->parse_header("[BAR]"), syntax_error);
+  EXPECT_THROW(parser->parse_header("[root]"), syntax_error);
+}
+// }}}

tests/unit_tests/utils/string.cpp

@@ -1,7 +1,5 @@
-#include <iomanip>
-
-#include "common/test.hpp"
 #include "utils/string.hpp"
+#include "common/test.hpp"
 
 using namespace polybar;
 
@@ -57,6 +55,11 @@ TEST(String, trim) {
   EXPECT_EQ("test", string_util::trim("xxtestxx", 'x'));
 }
 
+TEST(String, trimPredicate) {
+  EXPECT_EQ("x\t x", string_util::trim("\t  x\t x   ", isspace));
+  EXPECT_EQ("x\t x", string_util::trim("x\t x   ", isspace));
+}
+
 TEST(String, join) {
   EXPECT_EQ("A, B, C", string_util::join({"A", "B", "C"}, ", "));
 }